refactor(sdk): build dynamic system-message suffix via the prompt registry#3837
Conversation
…istry get_system_message_suffix now assembles the dynamic-tier sections through create_registry().build(ctx).dynamic instead of rendering the system_message_suffix.j2 template, which is deleted. The template's content was already ported verbatim into the registry's dynamic sections (#3610), so this removes the duplicate source of truth without changing the public API.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — well-scoped refactor that consolidates dynamic-tier rendering into the existing section registry without changing observable behavior.
Summary. The PR deletes context/prompts/templates/system_message_suffix.j2 and rewrites AgentContext.get_system_message_suffix (and the ACP path it now shares) to build the dynamic block via create_registry().build(ctx).dynamic. The five dynamic sections (DateTimeSection, RepoContextSection, AvailableSkillsSection, CustomSuffixSection, CustomSecretsSection) already exist in sections/dynamic.py; the change wires them into the same factory that powers the static block.
Equivalence check. Walked each section against the deleted .j2:
- Order: DateTime → RepoContext → AvailableSkills → CustomSuffix → CustomSecrets — preserved.
- Guards:
now/repo_skills/available_skills_prompt/custom_suffix.strip()/secret_infostruthiness — preserved. - Custom-suffix wrapping: Old template wrapped the user value with leading/trailing blank lines; new code returns
ctx.custom_suffixraw and the registry joins/strips at the outer level. Net output is byte-identical after the outer.strip(). - Empty-output
return None: when every section is guarded off,dynamicisNone— matches the oldreturn Nonebranch. - ACP path:
to_acp_prompt_contextnow delegates toget_system_message_suffix, eliminating the last duplicate render path — strictly an improvement.
Tests. No test logic changed, only a docstring in test_acp_agent.py. That's fine because coverage is real:
tests/sdk/context/prompts/test_prompt_snapshot.pypins the rendered dynamic block as golden files across the full model-family × browser × security-analyzer × cli-mode matrix, so any whitespace drift will fail.tests/sdk/context/test_agent_context.pyexercises each section (datetime-only, repo skills, available skills, custom suffix alone, secrets alone, and combinations).tests/sdk/context/test_agent_context_model_specific.pycovers the model-family gating preserved from the old template.
Risk. 🟢 LOW. Pure refactor; net source delta is −85/+25 lines; no public API change; no behavior change for callers.
Approving.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the SDK prompt-suffix path with real AgentContext calls on main and this PR; the PR preserves rendered dynamic context behavior while ACP continues delegating to the same output.
Does this PR achieve its stated goal?
Yes. The PR set out to remove the duplicated Jinja dynamic system-message suffix path and route AgentContext.get_system_message_suffix() / ACP prompt context through the registry without changing public behavior. Running the SDK against main and then commit 7ec9a845 showed the rich dynamic suffix still includes datetime, repo context, available skills, custom suffix, and custom secrets; to_acp_prompt_context() still returns the identical string; and canonicalized output matched the baseline exactly (d68ebaf1756697fc0a50e2d63832e132afc3aeef8401d5fa8f08853ab87ca36a).
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully and installed the uv-managed dev environment. |
| CI Status | 🟡 GitHub reported 35 passing checks, 0 failing checks, 1 pending check (qa-changes), and 2 skipped checks at review time. |
| Functional Verification | ✅ Real SDK function calls matched baseline behavior; ACP delegation and no-content edge cases were verified. |
Functional Verification
Test 1: Rich dynamic system-message suffix and ACP delegation
Step 1 — Establish baseline without the fix:
Ran git checkout origin/main && uv run python /tmp/qa_suffix_check.py using a real AgentContext containing a repo skill, keyword-triggered skill, custom suffix, static secret, additional secret info, and fixed datetime. Relevant output:
suffix_is_none False
acp_matches_suffix True
missing_fragments []
canonical_sha256 d68ebaf1756697fc0a50e2d63832e132afc3aeef8401d5fa8f08853ab87ca36a
canonical_line_count 51
raw_line_count 57
This shows the pre-PR SDK rendered all expected dynamic sections and ACP delegated to the same suffix output.
Step 2 — Apply the PR's changes:
Checked out 7ec9a845cac99b89bcf26c3ee46a31ad7f54aa82 (refactor(sdk): build dynamic system-message suffix via the prompt registry).
Step 3 — Re-run with the fix in place:
Ran uv run python /tmp/qa_suffix_check.py again. Relevant output:
suffix_is_none False
acp_matches_suffix True
missing_fragments []
canonical_sha256 d68ebaf1756697fc0a50e2d63832e132afc3aeef8401d5fa8f08853ab87ca36a
canonical_line_count 51
raw_line_count 52
This confirms the registry-backed path still renders the required user-visible prompt sections and ACP still receives identical prompt context. The canonical hash matches main; the raw line-count reduction reflects the PR-described blank-line normalization.
Test 2: No dynamic content and custom-only edge behavior
Step 1 — Establish baseline without the fix:
Ran git checkout origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_suffix_edge_check.py:
no_dynamic_content None
custom_no_datetime 'Custom only.'
This confirms the old path returned None when datetime and all dynamic inputs were absent, and trimmed a custom-only suffix.
Step 2 — Apply the PR's changes:
Checked out 7ec9a845cac99b89bcf26c3ee46a31ad7f54aa82.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_suffix_edge_check.py:
no_dynamic_content None
custom_no_datetime 'Custom only.'
This confirms the no-content and custom-only edge behavior remains unchanged after the registry migration.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
@enyst wdyt? |
3 participants
HUMAN:
Deleting the dynamic system promtp jinja.
AGENT:
Why
system_message_suffix.j2was the last prompt template still rendered through theJinja path, even though its content had already been ported verbatim into the typed
prompt registry's dynamic-tier sections (
context/prompts/sections/dynamic.py,#3610). That left two sources of truth for the same suffix, kept in sync only by
snapshot tests. The main Agent already builds its dynamic block from the registry
(
AgentBase.dynamic_context); onlyAgentContext.get_system_message_suffix— and theACP path that delegates to it — still rendered the template.
Summary
AgentContext.get_system_message_suffixnow assembles the dynamic-tier sections viacreate_registry().build(ctx).dynamicinstead of renderingsystem_message_suffix.j2. Public signature unchanged (no API break).openhands-sdk/openhands/sdk/context/prompts/templates/system_message_suffix.j2— the duplicate source of truth.Issue Number
Follow-up to the prompt-registry port (#3610).
How to Test
Run the test suites that cover the suffix/registry/ACP paths:
End-to-end render through the migrated path (not just unit tests) — build a context
with a repo skill, a triggered skill, a custom suffix, secrets, and a datetime:
Result: renders
<CURRENT_DATETIME>,<REPO_CONTEXT>,<SKILLS>, the custom suffix,and
<CUSTOM_SECRETS>(advertising$GITHUB_TOKEN);to_acp_prompt_context()returnsthe identical string.
Behavior-preservation vs
main— rendering the same context onmain(the.j2renderer) and on this branch (the registry) and diffing:
True).single blank line, while the old template's untrimmed
{% if %}blocks left 2-5. Thismatches what
AgentBase.dynamic_contextalready emitted, and the existing paritytests normalize it (
_canonical_gaps).No public API break —
griffe.find_breaking_changes(main, HEAD)foropenhands.sdkreports
0breaking changes (removed public class members: NONE), so theapi-breakagegate stays green.Video/Screenshots
N/A — internal prompt-assembly refactor with no UI surface. Rendered-output evidence is
under "How to Test".
Type
Notes
base.pystill saysget_system_message_suffix"renders"the string — left as-is to keep the diff minimal now that the method builds via the
registry; happy to tweak that one line on request.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:8d135e0-pythonRun
All tags pushed for this build
About Multi-Architecture Support
8d135e0-python) is a multi-arch manifest supporting both amd64 and arm648d135e0-python-amd64) are also available if needed