Skip to content

fix(channels/feishu,providers): wire mention_only config and disable native tools for Groq#5848

Open
singlerider wants to merge 4 commits intozeroclaw-labs:masterfrom
singlerider:fix/feishu-mention-only
Open

fix(channels/feishu,providers): wire mention_only config and disable native tools for Groq#5848
singlerider wants to merge 4 commits intozeroclaw-labs:masterfrom
singlerider:fix/feishu-mention-only

Conversation

@singlerider
Copy link
Copy Markdown
Collaborator

@singlerider singlerider commented Apr 17, 2026

Summary

Validation Evidence (required)

  • Commands run and tail output: cargo fmt --all -- --check and cargo clippy --workspace --all-targets -- -D warnings both clean.
  • Beyond CI — what did you manually verify? Verified that all FeishuConfig { ... } struct literals across the codebase have the new field. Fixed from_feishu_config() propagation bug (was hardcoding false) and added two-way test lark_from_feishu_config_propagates_mention_only to pin both values through the constructor. Diffed our changes against fix(channel,provider): Feishu mention_only ignored and Groq tool_use_failed 400 #5676 line-by-line to confirm full coverage.
  • If any command was intentionally skipped, why: Full local test run skipped — this is a maintainer-applied rebase of a contributor's already-reviewed and CI-green diff onto new file paths. CI will run the full battery.

Security & Privacy Impact (required)

  • New permissions, capabilities, or file system access scope? No
  • New external network calls? No
  • Secrets / tokens / credentials handling changed? No
  • PII, real identities, or personal data in diff, tests, fixtures, or docs? No

Compatibility (required)

  • Backward compatible? Yes — mention_only has #[serde(default)] so existing configs without the field deserialize to false (previous hardcoded behavior).
  • Config / env / CLI surface changed? Yes — mention_only is now a valid field under [channels.feishu] in zeroclaw.toml.
  • Upgrade steps: None required. Add mention_only = true to [channels.feishu] to opt in.

Rollback (required for risk: medium and risk: high)

  • Fast rollback command/path: git revert 552f0868 dbeeb94d
  • Feature flags or config toggles: None — mention_only defaults to false, preserving prior behavior on rollback.
  • Observable failure symptoms: Feishu bot responds to all group messages despite mention_only = true; Groq tool calls return 400 with tool_use_failed.

Supersede Attribution (required only when Supersedes # is used)

i18n Follow-Through

N.A. — no docs or user-facing wording changed.


@theonlyhennygod — your fix in #5676 was correct and complete. The only thing blocking it was merge conflicts from the workspace split. Rather than asking you to rebase across a large structural diff, I've ported your changes directly onto the new crate paths. #5676 will be closed when this merges. Thank you for tracking both bugs down.

@freeakanayaka — FYI on this one since it touches Groq tool-call behavior. The .without_native_tools() addition disables native tool schemas for Groq, routing through the text-based fallback instead. Let me know if that conflicts with anything you're aware of on the provider side.

…ve tools for Groq

- Add `mention_only` field to `FeishuConfig` in zeroclaw-config schema.
  Previously hardcoded to `false` in LarkChannel constructor, silently
  ignoring any user-set value.
- Wire `config.mention_only` through `LarkChannel::from_feishu_config`
  so group-message filtering actually respects the config field.
- Add `mention_only: false` to all `FeishuConfig` struct literals in
  tests across zeroclaw-channels, zeroclaw-config, zeroclaw-gateway,
  zeroclaw-runtime, and zeroclaw-tui.
- Add `.without_native_tools()` to Groq provider to fix
  `tool_use_failed` 400 errors on Groq endpoints that do not support
  the native tool-call protocol.

Supersedes zeroclaw-labs#5676 by @theonlyhennygod (rebased onto post-workspace-split
crate structure; original diff applied verbatim to new paths).
@github-actions github-actions bot added the config Auto scope: src/config/** changed. label Apr 17, 2026
Copy link
Copy Markdown
Collaborator

@WareWolf-MoonWall WareWolf-MoonWall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the full diff, both commit messages, and the current state of from_feishu_config() on master before writing this. Two things need to be resolved before this can merge.


🔴 Blocking — from_feishu_config() still hardcodes false; issue #5672 is not actually fixed

This is the most important finding, and I want to be precise about it so the fix is clear.

The commit message for 552f0868 says:

Wire config.mention_only through LarkChannel::from_feishu_config so group-message filtering actually respects the config field.

That bullet is not reflected in the actual diff. The combined delta for crates/zeroclaw-channels/src/lark.rs is 2 ++ — both additions are mention_only: false in test struct literals in mod tests. The body of from_feishu_config() itself is unchanged. On the current master, it reads:

let mut ch = Self::new_with_platform(
    config.app_id.clone(),
    config.app_secret.clone(),
    config.verification_token.clone().unwrap_or_default(),
    config.port,
    config.allowed_users.clone(),
    false,                     // ← still hardcoded
    LarkPlatform::Feishu,
);

If this PR merges as-is, FeishuConfig gains the mention_only field in the schema, but from_feishu_config() silently discards it. Users who set mention_only = true will see no change in behavior. Issue #5672 closes on paper and stays broken in practice.

The PR description says from_feishu_config() "already reads config.mention_only on master (landed separately)." I can see where the confusion came from: the old monorepo path src/channels/lark.rs does have config.mention_only in that position — it was wired in commit 8759af5e ("route Feishu through agent loop with tools and add mention_only"). But the workspace-split file at crates/zeroclaw-channels/src/lark.rs was initialized with the old false and that detail was never carried over. The rebase ported the schema field addition and the test literals, but missed the one-line fix in the constructor call.

The required change is a single character replacement at the mention_only argument in from_feishu_config()falseconfig.mention_only. It should also be accompanied by a test that actually exercises the mention_only = true path through from_feishu_config(), not just a struct literal that compiles.

For reference, the existing test lark_from_feishu_config_sets_feishu_platform at line 3085 asserts the platform is set correctly but never asserts parsed.mention_only == true when the config has it set. A test like that would have caught this gap in 4dfbfe5a and would catch any regression here.


🔴 Blocking — Missing Co-authored-by trailers

The PR discipline doc at docs/contributing/pr-discipline.md is unambiguous:

Always include Co-authored-by trailers for materially incorporated contributors, regardless of the circumstances that led to the supersede.

And:

In the integrating commit message, add one Co-authored-by: Name <email> trailer per superseded contributor whose work is materially incorporated.

The PR body acknowledges this is missing and gives the rationale: maintainer rebase, credit in the PR body and commit message body text. I understand the intent — the attribution in the PR body is genuinely more visible than a trailer to most people reading the thread — but that doesn't satisfy the doc's requirement. The reason Co-authored-by trailers exist as a technical requirement rather than a prose convention is that they wire into GitHub's contribution graph. @theonlyhennygod gets a green square for this work if the trailer is present. They do not if it isn't. That is a real and concrete difference, not a formality.

The fix is straightforward: amend 552f0868 to add the trailer and force-push. The format is:

Co-authored-by: Argenis De La Rosa <theonlyhennygod@users.noreply.github.com>

Use theonlyhennygod@users.noreply.github.com unless you have their verified commit email — GitHub will recognize the noreply address.


🟡 Conditional — without_native_tools() applies blanket to all Groq models; needs a tracked follow-up

The Groq fix is correct for the reported case. Llama-family models on Groq hallucinate function calls under the native tool protocol, and routing them through text-based tool use is the right resolution for now. The @freeakanayaka FYI in the description was the right call too.

The concern worth tracking is that this disables native tool schemas for all Groq models unconditionally. Groq's catalog is broader than llama, and if they surface a model that handles the native protocol correctly, users on that model will silently get the degraded path with no way to opt back in short of a config flag or a code change. It's also a subtle surprise for anyone who reads the provider list and wonders why Groq is treated differently from Mistral or the other OpenAI-compatible entries.

I don't want to hold up the bug fix on this — the 400 errors are actively breaking agents. But I'd like to see a tracked issue filed before merge that records: current behavior, the per-model detection path as a candidate solution, and a config escape hatch (native_tools: true/false under the Groq provider config) as an alternative. That issue should have an assignee so it doesn't get filed and forgotten. Once that issue is open and linked, I'm comfortable merging on the Groq side.


🟡 Conditional — Missing risk label

The original #5676 was labeled risk: medium. This PR covers the same changes plus carries them across more crate boundaries. The config surface change and the blanket provider behavior change for Groq both qualify for risk: medium under the AGENTS.md tier definitions. Please add the label before merge so the PR history is queryable by risk tier.


#[serde(default)] on mention_only is exactly right

Adding the field with #[serde(default)] means any existing zeroclaw.toml that omits mention_only under [channels.feishu] deserializes to false — identical to the previous hardcoded behavior. No breakage, no upgrade steps, full backward compatibility. This is how new optional config fields should always land.

✅ Struct literal coverage across all crates

Every FeishuConfig { ... } literal across zeroclaw-channels, zeroclaw-config, zeroclaw-gateway, zeroclaw-runtime, zeroclaw-tui, and the root crate was updated. That kind of completeness prevents the "compiles on one crate, silently wrong in another" class of bugs. The second commit catching the root crate miss is also a good sign — it means the author re-checked after the first push rather than assuming it was done.

✅ Honest validation narrative

Being explicit that cargo test was deferred to CI and why — rather than listing it as run when it wasn't — is the right behavior. The reasoning (maintainer rebase of a CI-green diff; full battery deferred to CI) is coherent and the PR description makes the tradeoff visible rather than hiding it.


Summary

Two things block merge:

  1. from_feishu_config() must be changed to pass config.mention_only instead of false. The schema field addition alone does not fix #5672. Add a test asserting mention_only propagates through from_feishu_config() to make this regression-proof.

  2. Co-authored-by: Argenis De La Rosa <theonlyhennygod@users.noreply.github.com> must be in the commit message of 552f0868. Amend and force-push.

Two conditionals before merge:

  1. File a tracked issue for per-model or configurable native tool support on Groq, with an assignee. Link it in this PR thread.

  2. Add risk: medium label.

The code changes that are in the diff are correct. The schema field, the serde default, the Groq fix, the literal coverage — all of it is clean. This just needs the one missing line in from_feishu_config(), the commit trailer, and the two housekeeping items above.

singlerider and others added 2 commits April 20, 2026 14:13
`from_feishu_config` was hardcoding `false` for the `mention_only`
parameter despite the config field being present. This meant all
Feishu channels constructed via this path silently ignored the
configured filter.

Fixes the propagation and adds a two-way test to pin both true and
false values through the constructor.

Co-authored-by: Argenis De La Rosa <theonlyhennygod@users.noreply.github.com>
@singlerider singlerider added the risk: medium Auto risk: src/** or dependency/config changes. label Apr 20, 2026
@singlerider singlerider dismissed WareWolf-MoonWall’s stale review April 20, 2026 05:03

All four items addressed: from_feishu_config propagation fixed with two-way test, Co-authored-by added, Groq follow-up filed as #5932, risk:medium label applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

config Auto scope: src/config/** changed. risk: medium Auto risk: src/** or dependency/config changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Feishu responds even when mention_only is enabled [Bug]: Groq provider 400 error

2 participants