Refactor instructions parsing #204
Conversation
WalkthroughThis PR adds a dedicated Instructions class that resolves Changes
Sequence Diagram(s)sequenceDiagram
participant Agent
participant Instructions
participant LLM
participant Plugin
Agent->>Instructions: Instructions(input_text, base_dir)
activate Instructions
Instructions->>Instructions: extract `@file.md` refs, read files, build full_reference
Instructions-->>Agent: full_reference
deactivate Instructions
Agent->>LLM: set_instructions(Instructions)
activate LLM
LLM->>LLM: _instructions = Instructions.full_reference
deactivate LLM
Plugin->>LLM: request (converse / realtime)
activate LLM
alt _instructions present
LLM-->>Plugin: provide _instructions as system message
else
LLM-->>Plugin: no system message
end
deactivate LLM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)plugins/openai/tests/test_openai_realtime.py (4)
plugins/gemini/tests/test_gemini_llm.py (3)
plugins/aws/tests/test_aws_realtime.py (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dangusev
force-pushed
the
fix/instructions-parsing
branch
from
f2e38e9 to
7deee3b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
agents-core/vision_agents/core/instructions.py (2)
12-12: Regex pattern may not handle all edge cases.The pattern
r"@([^\s@]+\.md)"matches@filename.mdbut may capture unintended strings. For example:
@path/to/file.md- captures the entire path (likely intended)@@file.md- doesn't match due to[^\s@](good)@file.md.backup- doesn't match due to.mdat end (good)@file.md,- capturesfile.md,including the commaConsider adding word boundary or punctuation handling if needed:
-_MD_PATTERN = re.compile(r"@([^\s@]+\.md)") +_MD_PATTERN = re.compile(r"@([^\s@,;:!?]+\.md)\b")
87-99: Ensure path traversal protection is robust.The security check on line 98 uses
is_relative_to(self._base_dir)which correctly prevents directory traversal attacks. However, the check order could be optimized: you checkis_file()beforeexists(), which is redundant sinceis_file()returnsFalseif the path doesn't exist.Consider reordering for clarity and efficiency:
# Check if the path is a file, it exists, and it's a markdown file. skip_reason = "" - if not full_path.is_file(): - skip_reason = "path is not a file" + if not full_path.exists(): + skip_reason = "file not found" elif full_path.name.startswith("."): skip_reason = "path is invalid" - elif not full_path.exists(): - skip_reason = "file not found" + elif not full_path.is_file(): + skip_reason = "path is not a file" elif full_path.suffix != ".md": skip_reason = "file is not .md" # The markdown file also must be inside the base_dir elif not full_path.is_relative_to(self._base_dir): skip_reason = "file outside the base directory"tests/test_instructions.py (2)
6-26: Parametrized success cases look solid; consider one case with differing file contentsThe parametrized
test_parse_successnicely covers empty, plain-text, and multi-file happy paths, and the expectedfull_referencematches the current_extract_full_referencebehavior (including the triple newline before## Referenced Documentation:). To guard against regressions where different files might accidentally share or overwrite content, you could add (or tweak) a case wherefile1.mdandfile2.mdcontain different text so that the mapping by filename is also implicitly checked.
55-65: Hidden.mdfiles produce a placeholder entry—confirm this matches the intended “ignore” semanticsThis test encodes that
@.file1.mdyields a### .file1.mdheading with the generic “File not found or could not be read” placeholder, even when the file exists. That matches_read_md_filereturning""for names starting with".", but it means hidden files are still surfaced in the Referenced Documentation section, just without contents. If “ignore files starting with '.'” is meant to hide them entirely (no section at all), you may want to adjust either the implementation or this expectation; otherwise, this test is a clear spec of the current behavior.agents-core/vision_agents/core/llm/llm.py (2)
27-31: Participant type hint now comes from getstream’s protobuf; consider aligning with the edgeParticipanttypeHere
Participantis imported fromgetstream.video.rtc.pb...models_pb2, whereas other parts of the system (e.g.,vision_agents.core.edge.typesand the OpenAI Realtime implementation) use a differentParticipanttype. Since this is only a type hint it won’t break runtime behavior, but if you care about static checking it might be worth standardizing on a single Participant abstraction (or using a Protocol/alias) so overrides ofsimple_response/simple_audio_responseremain type‑compatible.
377-388: Harden_sanitize_tool_outputagainst non‑JSON‑serializable valuesRight now,
_sanitize_tool_outputcallsjson.dumps(value)for all non‑string values, which will raiseTypeErrorfor common Python objects (e.g., datetimes, custom classes) and could turn tool execution into an unexpected failure at the sanitization step. Since this helper is meant to be defensive, it’s safer to fall back tostr()if JSON encoding fails or to usedefault=strinjson.dumps.You could make it more robust along these lines:
- s = value if isinstance(value, str) else json.dumps(value) + if isinstance(value, str): + s = value + else: + try: + s = json.dumps(value, default=str) + except TypeError: + # Fallback for objects json can't handle + s = str(value)This keeps the length‑limiting behavior but avoids crashing on rich tool results.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
agents-core/vision_agents/core/agents/agents.py(3 hunks)agents-core/vision_agents/core/instructions.py(1 hunks)agents-core/vision_agents/core/llm/llm.py(4 hunks)agents-core/vision_agents/core/utils/utils.py(0 hunks)docs/ai/instructions/ai-llm.md(1 hunks)docs/ai/instructions/ai-realtime-llm.md(1 hunks)plugins/aws/tests/test_aws.py(1 hunks)plugins/aws/tests/test_aws_realtime.py(2 hunks)plugins/aws/vision_agents/plugins/aws/aws_llm.py(2 hunks)plugins/aws/vision_agents/plugins/aws/aws_realtime.py(1 hunks)plugins/gemini/tests/test_gemini_llm.py(1 hunks)plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py(1 hunks)plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py(1 hunks)plugins/openai/tests/test_openai_realtime.py(1 hunks)plugins/openai/vision_agents/plugins/openai/chat_completions/chat_completions_llm.py(1 hunks)plugins/openai/vision_agents/plugins/openai/chat_completions/chat_completions_vlm.py(1 hunks)plugins/openai/vision_agents/plugins/openai/openai_llm.py(3 hunks)plugins/openai/vision_agents/plugins/openai/openai_realtime.py(2 hunks)plugins/openrouter/tests/test_openrouter_llm.py(1 hunks)tests/test_instructions.py(1 hunks)
💤 Files with no reviewable changes (1)
- agents-core/vision_agents/core/utils/utils.py
🧰 Additional context used
🧬 Code graph analysis (10)
plugins/openai/vision_agents/plugins/openai/openai_llm.py (3)
agents-core/vision_agents/core/edge/sfu_events.py (1)
Participant(229-270)agents-core/vision_agents/core/llm/llm.py (2)
LLM(50-388)LLMResponseEvent(39-43)agents-core/vision_agents/core/llm/llm_types.py (2)
NormalizedToolCallItem(107-111)ToolSchema(64-67)
plugins/aws/tests/test_aws_realtime.py (3)
plugins/openai/tests/test_openai_realtime.py (1)
realtime(20-30)agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
plugins/openai/vision_agents/plugins/openai/openai_realtime.py (2)
agents-core/vision_agents/core/instructions.py (1)
Instructions(15-115)agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)
plugins/gemini/tests/test_gemini_llm.py (2)
agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
plugins/openai/tests/test_openai_realtime.py (3)
plugins/aws/tests/test_aws_realtime.py (1)
realtime(20-33)agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
agents-core/vision_agents/core/llm/llm.py (4)
agents-core/vision_agents/core/instructions.py (1)
Instructions(15-115)agents-core/vision_agents/core/llm/events.py (2)
ToolEndEvent(126-135)ToolStartEvent(116-122)agents-core/vision_agents/core/agents/agents.py (1)
Agent(75-1317)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
plugins/aws/tests/test_aws.py (2)
agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
agents-core/vision_agents/core/agents/agents.py (1)
agents-core/vision_agents/core/instructions.py (1)
Instructions(15-115)
tests/test_instructions.py (1)
agents-core/vision_agents/core/instructions.py (1)
Instructions(15-115)
plugins/openrouter/tests/test_openrouter_llm.py (2)
agents-core/vision_agents/core/llm/llm.py (1)
set_instructions(179-180)plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
set_instructions(471-473)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit / Test "not integration"
- GitHub Check: unit / Test "not integration"
🔇 Additional comments (23)
plugins/gemini/vision_agents/plugins/gemini/gemini_realtime.py (1)
311-336: LGTM!The change from
_build_enhanced_instructions()to direct use ofself._instructionscorrectly consolidates instruction handling. This aligns with the new Instructions abstraction wherefull_referenceis precomputed and stored viaset_instructions.docs/ai/instructions/ai-realtime-llm.md (1)
49-50: LGTM!The documentation example correctly demonstrates the simplified instruction handling pattern using
self._instructionsdirectly.docs/ai/instructions/ai-llm.md (1)
25-26: LGTM!Documentation example is consistent with the new instruction handling approach.
plugins/aws/vision_agents/plugins/aws/aws_llm.py (2)
130-131: LGTM!Correctly replaced enhanced instruction construction with direct use of
self._instructionsin theconversemethod.
356-357: LGTM!Correctly replaced enhanced instruction construction with direct use of
self._instructionsin theconverse_streammethod, consistent with theconversemethod.agents-core/vision_agents/core/instructions.py (1)
107-115: LGTM!The file reading implementation correctly handles potential errors with appropriate exception handling and logging.
plugins/openai/vision_agents/plugins/openai/chat_completions/chat_completions_llm.py (1)
170-171: LGTM!Correctly migrated from public
instructionsattribute to private_instructionsattribute, consistent with the new Instructions API pattern.plugins/aws/vision_agents/plugins/aws/aws_realtime.py (1)
232-236: LGTM! Cleaner instruction handling.The refactor simplifies the flow by directly using
self._instructionsinstead of calling a separate_build_enhanced_instructions()method. The explicit check ensures instructions are set before connection, which is good defensive programming for AWS Bedrock's requirements.plugins/aws/tests/test_aws.py (1)
149-149: LGTM! Public API usage.Good migration from the private
_set_instructionsto the publicset_instructionsAPI. This aligns with the broader refactor introducing standardized instruction handling.plugins/gemini/vision_agents/plugins/gemini/gemini_llm.py (1)
165-165: LGTM! Simplified initialization.The change removes the indirect
_build_enhanced_instructions()call in favor of directly usingself._instructionsas the system instruction. This is cleaner and aligns with the PR's goal of consolidating instruction parsing logic into theInstructionsclass.agents-core/vision_agents/core/agents/agents.py (2)
29-29: LGTM! Introduction of Instructions abstraction.The Agent now wraps the instruction string in an
Instructionsobject, which parses markdown file references (like@file.md) and resolves them into afull_reference. This centralizes instruction parsing logic as intended by the PR.Also applies to: 143-143
510-510: LGTM! Proper use of full_reference.When passing instructions to
edge.create_conversation, the code correctly extractsself.instructions.full_reference(the composed string with resolved markdown) rather than passing the Instructions object itself. This maintains a clean interface.plugins/gemini/tests/test_gemini_llm.py (1)
87-87: LGTM! Public API migration.Correctly migrated from the private
_set_instructionsto the publicset_instructionsmethod, consistent with the broader refactor.plugins/openai/tests/test_openai_realtime.py (1)
26-26: LGTM! Public API adoption.Test correctly uses the public
set_instructionsmethod instead of the private_set_instructions, aligning with the standardized instruction handling across the codebase.plugins/openai/vision_agents/plugins/openai/openai_llm.py (2)
101-101: LGTM! Streamlined instruction handling.The refactor removes complex conditional logic for building enhanced instructions and instead relies directly on
self._instructions. This is much cleaner and aligns with the centralizedInstructionsclass approach. The instruction flow is now: Agent wraps string in Instructions → set_instructions extracts full_reference → LLM uses it directly.Also applies to: 134-136
4-4: No issues found with public API surface.The newly imported types (
Participant,OpenAIResponse,NormalizedToolCallItem,ToolSchema) are used internally within the module and in method signatures, but they are not re-exported inplugins/openai/vision_agents/plugins/openai/__init__.py. OnlyOpenAILLMis exposed as the public API (aliased asLLM). Using types in method signatures is standard Python practice and does not constitute an accidental API expansion, as users interact with theOpenAILLMclass directly without needing to import these types themselves.plugins/openai/vision_agents/plugins/openai/chat_completions/chat_completions_vlm.py (1)
248-252: No issues found—API change is safe.Verification confirms that no external code, test files, or other parts of the codebase access the public
instructionsattribute onChatCompletionsVLM. The refactoring fromself.instructions(public) toself._instructions(private) introduces no public API breakage.tests/test_instructions.py (3)
27-46: Good coverage of “not a file” vs “missing file” fallback behaviorThese two tests clearly assert that both a directory at
file1.mdand a non-existentfile1.mdyield the same placeholder*(File not found or could not be read)*in the Referenced Documentation section, which matches the current_read_md_filebehavior (returning""for any non-file path). This is a useful contract to pin down so future refactors don’t accidentally raise or silently drop these references.
47-54: Non‑markdown references correctly remain untouched
test_parse_file_not_mdensures that@file1.txtdoes not trigger any Referenced Documentation section and thatfull_referenceis exactly the originalinput_text. This aligns with the_MD_PATTERNbehavior (only*.mdis matched) and is a good regression test for “don’t over‑interpret arbitrary@tokens.”
66-81: Outside‑base‑dir absolute path handling matches design
test_parse_file_outside_base_dirnicely nails the edge case where the user mentions an absolute path outsidebase_dir: content from that path is intentionally not read (due to theis_relative_to(self._base_dir)guard), but the original path string is preserved in the heading with the generic “could not be read” placeholder. This is a good, explicit test for the “don’t read outside base_dir, but keep the reference visible” policy.plugins/openai/vision_agents/plugins/openai/openai_realtime.py (1)
37-37: Realtime now correctly propagatesInstructions.full_referenceinto the OpenAI sessionOverriding
set_instructionsto accept anInstructionsinstance, delegate to the base LLM, and then assignself.realtime_session["instructions"] = self._instructionskeeps the public API uniform while ensuring the composedfull_referenceactually reaches the OpenAI Realtime session. This matches the new Instructions‑centric flow and avoids duplicating parsing logic here.Also applies to: 471-473
agents-core/vision_agents/core/llm/llm.py (2)
19-22: Instructions plumbing via_instructionsandset_instructionsis coherent and centralizes parsingImporting
Instructions, initializingself._instructionsonce in__init__, invokingself.set_instructions(agent.instructions)in_attach_agent, and implementingset_instructionsto storeinstructions.full_referencegives you a single, consistent place where raw agent instructions are transformed into the composed reference string. Subclasses (like provider‑specific LLMs) can rely on_instructionswithout worrying about parsing details, and the Agent still retains the fullInstructionsobject if anyone needs richer access.Also applies to: 56-65, 160-166, 179-181
242-315: Tool execution path with events, threading, and timeouts looks robust
_run_one_tooland_execute_toolsdo a good job of:
- Normalizing arguments from either
"arguments_json"or"arguments".- Fetching callables via
function_registry.get_callablewhen available, and offloading sync callables toasyncio.to_threadto avoid blocking the loop.- Wrapping execution in
asyncio.wait_forfor per‑tool timeouts.- Emitting
ToolStartEventandToolEndEventwith arguments, success flag, result/error, and execution time, which should be very helpful for observability.
The deduplication logic in_dedup_and_executevia_tc_keyis also straightforward and avoids redundant runs.Also applies to: 316-375
Code for parsing instructions is scattered around the repo, even though it's the same.
To fix that:
vision_agents.core.instructions.Instructionsclass that holds initial prompt and parses@markdown referencesSummary by CodeRabbit
New Features
Documentation
Tests