fix(agent): tell the model when to act and stop offering ids it must not fill (CRM-238) - #51
Conversation
…not fill (CRM-238)
Three prompt/schema defects behind "the AI does not move the card". They are the
upstream cause of CRM-237, whose server-side guard stays as the safety net.
1. The schema contradicted the prompt. The instruction said the conversation id
was "automatically extracted from the context" while the tool docstring —
which becomes the schema the model reads — offered it as a fillable
parameter ("optional, auto-extracted"). The model filled it with the CONTACT
id and the CRM answered 400 CONVERSATION_NOT_FOUND. Both ids are now marked
DO NOT SET / ignored.
Note the docstring is REWRITTEN at runtime (pipeline_manipulation.__doc__ =
f"""...""" further down the factory), so the static one the reader sees first
is discarded — the runtime one is what the model gets, and it had to be fixed
there too. The unit test caught this.
2. The prompt never said whether the conversation already had a card, so the
model chose add_to_pipeline for one that was already in the funnel. It now
states move_to_stage as the normal case for an ongoing conversation, and
add_to_pipeline only for a conversation not yet in any pipeline.
3. The instruction was two prohibitions in a row ("apply a rule only when…",
"do not move … without a matching rule") with no positive criterion; in one
live run the model called nothing at all. It now says WHEN TO ACT — when the
customer's message matches a stage's "move here when", call the tool with
that stage_id, and acting is expected — while keeping the guardrail that no
stage may be invented and that an unmatched situation leaves the card alone.
The instruction block moved into _pipeline_tool_instruction() so it can be
tested as rendered text instead of scraped from the source.
Live check after rebuilding the container, third contact, first attempt: the
card moved Qualificado -> Fechado in ~12s.
Tests: 9 examples in tests/unit/test_pipeline_prompt_guidance.py (positive
criterion, action choice, ids not the model's to fill, and the schema agreeing
with the prompt). Suite: 290 passed vs 266 on the clean develop baseline
(+24 = 8 CRM-235 + 7 CRM-237 + 9 here); the 3 failures and 7 collection errors
are identical to the baseline and pre-existing.
Reviewer's GuideRefactors the pipeline manipulation agent instruction into a reusable helper, tightens the tool schema and documentation so the model must not provide conversation/contact IDs, and adds tests asserting the rendered prompt text provides clear, positive guidance on when and how to manipulate pipeline cards. Sequence diagram for pipeline card movement guidancesequenceDiagram
participant Context as ConversationContext
participant Agent as LLM_Agent
participant Tool as pipeline_manipulation
participant CRM as CRM
Context->>Agent: Provide current conversation and pipeline rules
Agent->>Agent: Match customer message to a configured stage
alt Existing pipeline card
Agent->>Tool: pipeline_manipulation(action="move_to_stage", pipeline_id, stage_id)
Tool->>Context: Read conversation_id and contact_id
Tool->>CRM: Move current conversation card
else No pipeline card
Agent->>Tool: pipeline_manipulation(action="add_to_pipeline", pipeline_id, stage_id)
Tool->>Context: Read conversation_id and contact_id
Tool->>CRM: Add current conversation card
else No matching stage
Agent-->>Context: Leave card unchanged
end
Flow diagram for pipeline tool action selectionflowchart TD
A[Customer message matches a configured stage] --> B{Conversation already has a card?}
B -->|Yes| C[Call pipeline_manipulation with action move_to_stage]
B -->|No| D[Call pipeline_manipulation with action add_to_pipeline]
C --> E[Provide pipeline_id and stage_id]
D --> E
E --> F[conversation_id and contact_id come from context]
G[No stage matches] --> H[Leave the card where it is]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/services/adk/agents/llm_agent_builder.py" line_range="195-197" />
<code_context>
+ "description below, call the tool with that stage's stage_id. Acting is expected "
+ "in that case — do not wait for the customer to ask to be moved.\n"
+ "WHICH ACTION: use action=\"move_to_stage\" for a conversation that already has a card "
+ "in the pipeline — the normal case for an ongoing conversation. Use "
+ "action=\"add_to_pipeline\" only for a conversation that is not in any pipeline yet; "
+ "if a move fails because the card does not exist, then add it.\n"
+ "IDS: do NOT set conversation_id or contact_id — they come from the conversation "
+ "context and anything you pass is ignored. Provide pipeline_id and stage_id.\n"
</code_context>
<issue_to_address>
**issue (bug_risk):** The instruction explains which action corresponds to an existing or absent card, but it never tells the model whether the current conversation actually has a card. When that status is not present elsewhere in the model context, the model still cannot distinguish `move_to_stage` from `add_to_pipeline` and can choose `add_to_pipeline` for an existing card—the exact failure this change claims to fix.
**Triggers:** When the current conversation's pipeline-card status is not injected into the prompt or tool context visible to the model.
**Suggested fix:** Include the current card/pipeline status in the rendered instruction, or have the tool expose a status before requiring the model to choose between the two actions.
</issue_to_address>
### Comment 2
<location path="src/services/adk/tools/evo_crm/pipeline_manipulation.py" line_range="147-151" />
<code_context>
- 'complete_task': Mark a task as completed
- contact_id: ID of the contact (optional, auto-extracted from context)
- conversation_id: ID of the conversation (optional, auto-extracted from context)
+ contact_id: DO NOT SET. The contact of the current conversation is
+ taken from the context; any value passed here is ignored.
+ conversation_id: DO NOT SET. The current conversation is taken from
+ the context; any value passed here is ignored. (CRM-238: this
+ field used to read as "auto-extracted from context" while still
+ being offered as a parameter — the model filled it with the
+ CONTACT id and the CRM answered 404/400.)
</code_context>
<issue_to_address>
**issue (bug_risk):** The schema still declares `contact_id` and `conversation_id` as callable function parameters, and the implementation still accepts model-supplied values when context metadata is absent. Marking them `DO NOT SET` in the docstring discourages filling them but does not remove them from the schema or prevent a model-generated id from being used outside a populated conversation context.
**Triggers:** When the tool is invoked without `tool_context` ids or with context metadata that does not contain the relevant id.
**Suggested fix:** Remove these ids from the model-facing function schema and obtain them exclusively from context, or reject any non-context value instead of falling back to the model argument.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if the guidance is wrong, the agent could add conversations to the wrong pipeline or move cards to an incorrect stage, leaving incorrect CRM state after the prompt is reverted. Those changes are bounded and can be corrected or recomputed, but reverting the code does not undo pipeline mutations already made.
Blocking findings: src/services/adk/agents/llm_agent_builder.py:197, src/services/adk/tools/evo_crm/pipeline_manipulation.py:151
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "in the pipeline — the normal case for an ongoing conversation. Use " | ||
| "action=\"add_to_pipeline\" only for a conversation that is not in any pipeline yet; " | ||
| "if a move fails because the card does not exist, then add it.\n" |
There was a problem hiding this comment.
issue (bug_risk): The instruction explains which action corresponds to an existing or absent card, but it never tells the model whether the current conversation actually has a card. When that status is not present elsewhere in the model context, the model still cannot distinguish move_to_stage from add_to_pipeline and can choose add_to_pipeline for an existing card—the exact failure this change claims to fix.
Triggers: When the current conversation's pipeline-card status is not injected into the prompt or tool context visible to the model.
Suggested fix: Include the current card/pipeline status in the rendered instruction, or have the tool expose a status before requiring the model to choose between the two actions.
| contact_id: DO NOT SET. The contact of the current conversation is | ||
| taken from the context; any value passed here is ignored. | ||
| conversation_id: DO NOT SET. The current conversation is taken from | ||
| the context; any value passed here is ignored. (CRM-238: this | ||
| field used to read as "auto-extracted from context" while still |
There was a problem hiding this comment.
issue (bug_risk): The schema still declares contact_id and conversation_id as callable function parameters, and the implementation still accepts model-supplied values when context metadata is absent. Marking them DO NOT SET in the docstring discourages filling them but does not remove them from the schema or prevent a model-generated id from being used outside a populated conversation context.
Triggers: When the tool is invoked without tool_context ids or with context metadata that does not contain the relevant id.
Suggested fix: Remove these ids from the model-facing function schema and obtain them exclusively from context, or reject any non-context value instead of falling back to the model argument.
…RM-238)
Code review follow-ups on top of the CRM-238 fix.
The prompt and both docstrings claimed "anything you pass is ignored", but the
tool resolves `context_id or model_id`: the model's value still wins when the
context is silent, which test_pipeline_tool_context_ids asserts on purpose. The
wording now says the context supplies the ids and overrides what is passed.
Three tool errors told the model to fill exactly what the prompt forbids
("conversation_id is required to ..."). They now report the missing context, so
nothing invites the model to break the contract it was just given.
The no-rules branch kept the wording the schema no longer agrees with and got
none of the new guidance; it now carries the move-vs-add rule and the DO NOT SET.
A legacy flat rule rendered its stage without a stage_id, leaving the model with
nothing legal to pass while the instruction demands one and forbids inventing.
Note the shape has no known producer: neither UI emits a rule with the stage on
the rule itself.
Comments state the rule, not the incident, and the static docstring now says it
is replaced at the end of the factory - the trap that made the first patch a
no-op.
Tests: 310 passed, against 299 on this PR's base.
test_the_tool_schema_carries_the_same_rule guards the move-vs-add rule against
drift between the two files where it is written;
test_legacy_flat_rule_exposes_its_stage_id covers the render.
The branch was cut before ec4e6b3 (CRM-235) and 4286e76 (CRM-237) landed, so develop moved under it. Conflicts resolved in develop's favour where the two overlap: - _format_pipeline_rules_for_prompt: develop DROPS a stage with no stageId and a flat rule without one, instead of rendering the id-less line this branch was merely completing. A stage the tool cannot be called for has no business in the prompt, so develop's version wins and the review fix that only emitted the legacy id is dropped, along with its test - test_legacy_rule_without_an_id_is_ dropped now covers the shape. - The allow_pipeline_manipulation block keeps develop's guard (rules that render to nothing fall back to the generic text) and calls _pipeline_tool_instruction inside it, so the CRM-238 wording survives the restructure. The generic fallback now serves more cases than before, which makes the aligned no-rules text from this branch matter more, not less. Tests: 311 passed, 301 on develop; the 2 failures and 1 collection error are identical to the baseline and pre-existing.
Problema
Três defeitos de prompt/schema por trás de "a IA não move o card". São a causa upstream do CRM-237 — o guard de servidor de lá continua como rede de segurança, mas o problema nasce aqui.
1. O schema contradizia o prompt
A instrução dizia que o id vinha do contexto:
Enquanto a docstring da tool — que vira o schema que o modelo lê — oferecia o campo:
O modelo vê campo preenchível e preenche. Mandou o id do contato e o CRM respondeu
400 CONVERSATION_NOT_FOUND. Agora os dois ids sãoDO NOT SET ... ignored.Detalhe que o teste pegou: a docstring é reescrita em runtime (
pipeline_manipulation.__doc__ = f"""..."""mais abaixo na factory). A estática, que o leitor encontra primeiro, é descartada — o modelo recebe a dinâmica. Meu primeiro patch corrigiu só a estática e não teria efeito nenhum; o testetest_tool_schema_agrees_with_the_promptfalhou e expôs isso.2. O prompt nunca dizia se a conversa já tinha card
Sem essa informação o modelo escolhia no escuro — e escolheu
add_to_pipelinepara uma conversa já no funil. Agora a instrução diz:move_to_stageé o caso normal de conversa em andamento;add_to_pipelinesó para conversa fora de qualquer funil.3. A instrução era só proibição
Duas negativas seguidas, nenhum critério de quando agir. Numa execução ao vivo o modelo não chamou ferramenta alguma, com as regras corretamente presentes. Agora há um WHEN TO ACT explícito ("acting is expected", "do not wait for the customer to ask"), preservando o guardrail de não inventar estágio e de deixar o card parado quando nada casa.
Estrutura
O bloco saiu para
_pipeline_tool_instruction(), para ser testado como texto renderizado em vez de raspado do código-fonte — a primeira versão do teste liainspect.getsourcee quebrava conforme onde a concatenação de strings do Python cortava a linha.Verificação ao vivo
Container rebuildado, terceiro contato, primeira tentativa:
Antes deste PR, a mesma situação produziu: uma execução sem chamada de tool, e outra com
add_to_pipeline+ id errado.Testes
tests/unit/test_pipeline_prompt_guidance.py— 9 examples: critério positivo, escolha da ação, ids fora do alcance do modelo e o schema concordando com o prompt.Baseline: 290 passed contra 266 no
developlimpo — delta+24(8 do CRM-235 + 7 do CRM-237 + 9 daqui). As 3 falhas e 7 erros de coleta são idênticos ao baseline e pré-existentes.Sobre a prova negativa, com honestidade: removendo o fix, o arquivo de teste falha na coleta (o import de
_pipeline_tool_instructionnão resolve), não em asserção. Prova que o teste não passa sem o fix, mas não exercita as asserções contra o texto antigo — diferente das provas negativas do CRM-235 e do CRM-237, que falham por assertion.Base deste PR
Encadeado em
fix/CRM-237-context-ids-win(PR #50), que por sua vez sai do #49. Ordem de merge: #49 → #50 → este.Trade-off assumido
A instrução ficou mais longa e mais diretiva ("acting is expected"). Isso aumenta a chance de o agente mover o card — que é o objetivo — e, no limite, o risco de mover quando a correspondência é fraca. Mitigado por manter explícito que só valem os estágios listados e que situação sem correspondência deixa o card onde está. Se aparecer movimentação indevida, o ajuste é no texto das instruções por estágio, que é o que o operador controla.
Summary by Sourcery
Improve pipeline agent guidance so it acts on matching stage rules, selects the correct pipeline action, and does not attempt to provide context-derived IDs.
Bug Fixes:
Enhancements:
Tests: