test(integration): cover triggered skill and path-rule content across condensation - #4565
Conversation
… condensation Adds c06 and c07 for issue OpenHands#4544, plus the agent_context hook they need. Both run the whole scenario end to end against a real LLM: a keyword skill (c06) or a path rule (c07) activates outside the condenser's keep_first prefix, the agent does real work, a real summarizing condensation runs, and the trigger fires again. The asserted invariant is the one the issue asks maintainers to confirm: after condensation, triggered content is either still in the active view or recoverable when the trigger fires again. Preserve, reconstruct and reactivate all satisfy it, so the tests do not prejudge the mechanism. They FAIL on current main by design and the failure message is the diagnosis. The c* suite is optional and non-blocking, and a live run of the scenario is what settles whether re-inclusion on a fresh trigger is intended. Verdicts are three-way: mechanism preserved / recovered -> PASS marker in rendered text only -> FAIL, SUMMARY-ECHO ONLY neither -> FAIL, OpenHands#4544 REPRODUCED The middle one matters. Injected guidance is in the agent's context by design, so a strong model quotes it back, that message reaches the summarizer, and the summary carries the marker through. It reads as survival but is a coin flip: before this distinction existed, c07 went 2 PASS / 4 FAIL across six identical Opus runs. Presence is therefore measured only in the injection channel (extended_content), matching the issue's ask that content be retained or recovered DETERMINISTICALLY. The middle verdict then demonstrates the "permitted, not guaranteed" behavior rather than asserting it. Three further details that keep the tests honest: - c06's sentinel sits PAST N_CHAR_PREVIEW (500), not merely inside a longer body. __str__ truncates the TAIL, so a front-loaded marker survives, reaches the summarizer, and gets preserved. An earlier draft did exactly that and passed against a real model while the bug was live. - Presence is never measured with str(event). ObservationEvent.__str__ omits extended_content entirely, so that string can never contain path-rule text and would report "gone" regardless of truth. The rendered string is the summarizer's input; it is not the agent's context. - verify_result checks preconditions first and reports SCENARIO NOT SET UP separately from a verdict. In particular the carrying event must actually be absent from the view after condensation; if it survived, the content was never at risk and the run says so instead of pronouncing on the contract. BaseIntegrationTest gains an overridable agent_context property defaulting to None, mirroring the existing tools/condenser hooks. Without it the harness builds Agent() with no context and skills cannot be installed at all. Verified on upstream/main (1de2e6d): ruff check and format clean, pyright clean, 27 passed / 1 skipped in tests/integration. Live runs on GLM-5.3 and Claude Opus 5; every run fails the contract, and PASS/FAIL no longer flaps. The verdict does still vary between the two failure modes: six c07 runs on Opus 5 gave 4 SUMMARY-ECHO ONLY and 2 OpenHands#4544 REPRODUCED. That split is the "permitted, not guaranteed" behavior itself, not instability in the test.
|
📁 PR Artifacts Notice This PR contains a |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Test-only PR adding two condenser-suite integration tests (c06, c07) that reproduce issue #4544 (triggered skill/path-rule content silently lost after condensation), plus a clean agent_context hook on BaseIntegrationTest.
Risk Assessment: LOW
No production code is touched. Both tests live in the optional, non-blocking c* suite and are designed to FAIL on current main — the requested artifact per the issue acceptance criteria.
Findings
No material bugs, security issues, or design flaws found. The tests are well-constructed:
base.py: Theagent_contextproperty addition mirrors the existingtoolsandcondenseroverride hooks and is correctly wired into theAgent()constructor.- c06 (keyword skill): Sentinel placement past
N_CHAR_PREVIEW(500) verified — at position 1072, soMessageEvent.__str__()truncation guarantees the marker never reaches the summarizer. Keyword injection correctly flows throughsend_message()→get_user_message_suffix()withskip_skill_names=activated_knowledge_skills, matching the production dedup path. - c07 (path rule): Uses the default tool preset (file editor included) so injection happens through the production
_maybe_inject_path_rules()callback. Verified thatObservationEvent.__str__()omitsextended_contentand theSkillvalidator forcesdisable_model_invocation=TrueforPathTrigger— both claims in the test rationale are accurate. - Three-way verdict system (PASS / SUMMARY-ECHO ONLY / REPRODUCED) is a sound design that avoids false positives from summary echoes while not prejudging the fix mechanism.
- Precondition checks in
verify_result(carrier event actually forgotten, scenario actually set up) prevent both false passes and false reproductions.
All technical claims in the PR description were verified against the source code.
HUMAN:
Human reviewed. These tests should help make the skill condensation behavior known
AGENT:
Why
#4544 reports that keyword-triggered skills and path-triggered rules are injected once per conversation, on an event condensation is allowed to forget, while the "already activated" marker lives in persistent state and is never cleared. After a condensation the guidance can be gone from everything the model sees while state still reports it as active.
The issue deliberately did not prescribe a mechanism, and asked maintainers to confirm the intended lifecycle instead. These tests run the whole scenario end to end so that question can be settled against observed behavior rather than against a reading of the code.
#4552 proposes a fix for the same issue at the session level. These tests sit a layer above it, in the live
c*integration suite it does not touch, so they are complementary rather than competing.Summary
c06_triggered_skill_survives_condensation.py: keyword-triggered skill, activated outsidekeep_first, real work, real summarizing condensation, then the keyword again.c07_path_rule_survives_condensation.py: the same shape for aPathTriggerrule, driven by real file edits through the file editor so injection happens in the production callback.BaseIntegrationTestgains an overridableagent_contextproperty defaulting toNone, mirroring the existingtoolsandcondenserhooks. Without it the harness buildsAgent()with no context and no skill can be installed.Both assert one invariant: after condensation, triggered content is either still in the active view or recoverable when the trigger fires again. Preserve, reconstruct and reactivate all satisfy it, so neither test prejudges the fix.
They fail on current main by design, and the failure message is the diagnosis. The
c*suite is optional and non-blocking, so a red case here does not gate anything.Issue Number
#4544
How to Test
Run against any OpenAI-compatible endpoint:
Observed on
anthropic/claude-opus-5, against1de2e6d1b:c07fails on every run, but lands on either of two verdicts. Across six runs on the same model it gaveSUMMARY-ECHO ONLYfour times and#4544 REPRODUCEDtwice:Both verdicts are the same failure. The injection channel is empty either way, so the contract fails; which one you land on depends on whether the model happened to quote the guidance back before condensation. Expect either when running it.
Also run on
glm-5.3.Local checks on the committed tree:
ruff checkandruff format --checkclean,pyrightclean,pytest tests/integration/27 passed and 1 skipped.Video/Screenshots
Type
Notes
Three things in the tests are deliberate and easy to undo by accident.
Verdicts are three-way, not two. A marker found only in rendered text gets its own
SUMMARY-ECHO ONLYverdict, which is a failure. Injected guidance sits in the agent's context by design, so a capable model quotes it back, that message reaches the summarizer, and the summary carries the marker through. It reads as survival but is a coin flip: before this distinction existed,c07went 2 pass / 4 fail across six identical runs on the same model. Presence is therefore measured only in the injection channel (extended_content), which matches the issue's ask that content be retained or recovered deterministically.c06's sentinel sits pastN_CHAR_PREVIEW(500), not merely inside a longer body.__str__truncates the tail, so a front-loaded marker survives, reaches the summarizer, and gets preserved. An earlier draft did that and passed while the bug was live.Presence is never measured with
str(event).ObservationEvent.__str__omitsextended_contententirely, so that string can never contain path-rule text and would report "gone" regardless of truth. The rendered string is what the summarizer is fed; it is not what the agent sees.verify_resultalso checks preconditions first and reportsSCENARIO NOT SET UPseparately from any verdict. In particular the carrying event must actually be absent from the view after condensation. If it survived, nothing was at risk and the run says so rather than pronouncing on the contract.