Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(test): repair four unit tests drifting behind source and upstream APIs by jariy17 · Pull Request #610 · aws/bedrock-agentcore-sdk-python · GitHub
Skip to content

fix(test): repair four unit tests drifting behind source and upstream APIs - #610

Merged
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift
Jul 31, 2026
Merged

fix(test): repair four unit tests drifting behind source and upstream APIs#610
jariy17 merged 1 commit into
mainfrom
fix/unit-test-drift

Conversation

@jariy17

Copy link
Copy Markdown
Contributor

Draft. Depends on #609 — see Ordering below.

Fixes the 4 pre-existing unit-test failures on main that #609 does not cover. All four are tests (or a message/doc) that fell behind source and upstream changes; none are product bugs.

1 + 2. TestAsyncMode multi-agent and bidi callbacks

test_agentcore_memory_session_manager.py:3726 and :3756 reached into the private registry dict:

callbacks=registry._registered_callbacks.get(event_type, [])
assertall(asyncio.iscoroutinefunction(cb) forcbincallbacks) # always False

Upstream Strands now stores callbacks wrapped in a _CallbackEntry:

type= <class 'strands.hooks.registry._CallbackEntry'>
repr= _CallbackEntry(callback=<function ...register_hooks.<locals>._offload.<locals>._callback ...>)

iscoroutinefunction() on the wrapper is always False, so the assertion could never hold. The source is correct — session_manager.py:989-991 does register async _offload callbacks.

The fix is the accessor the passing sibling tests already use (lines 3636–3714): public registry.get_callbacks_for(event), which unwraps.

Side benefit: the bidi-init assertion at :3751 is not any(iscoroutinefunction(...)). It was passing for the wrong reason — the wrapper is never a coroutine function, so it would have passed even if the callback were wrongly async. Against unwrapped callbacks it now actually tests what it claims.

3. EvaluatorOutput validator message — source fix

The message advertised a value escape hatch that does not exist:

ifnotself.errorCodeandself.labelisNone: # value is never inspectedraiseValueError("Either label, value, or errorCode must be set; ...")

The class docstring (models.py:63-65) and every caller in src/ agree that label is required unless errorCode is set, so the message was the defect, not the logic. Corrected to "label is required for success responses; ...", which is also what the test expected.

4. test_evaluation_with_empty_trajectory

TypeError: 'EvaluationReport' object is not subscriptable

run_evaluations() returns a single report now, verified against the installed library:

run_evaluations -> <class 'strands_evals.types.evaluation_report.EvaluationReport'>
model_fields: ['overall_score', 'scores', 'cases', 'test_passes', ...]

Dropped the [0]. Also fixed the same stale pattern in README.md (2 occurrences) — the documented reports = ...; report = reports[0] snippet would hand users the identical TypeError.

Testing

  • uv run pytest tests/ on Python 3.10 → 2934 passed, 10 skipped, 4 xpassed, 0 failed
  • TestAsyncMode in isolation → 8 passed
  • ruff check src/ tests/ → all checks passed; ruff format applied (the shortened message now fits one line — verified main was format-clean beforehand, so that reflow is mine)

Ordering

This branch is cut from main, so on its own CI still hits the 2 deepeval collection errors that #609 fixes — verified with a clean uv sync --dev:

ERROR tests/.../third_party/deepeval/test_adapter.py
ERROR tests/.../third_party/deepeval/test_error_handling.py
Interrupted: 2 errors during collection

Kept as a draft for that reason. #609 then this turns ci.yml green; merge order the other way leaves 4 failures. Happy to rebase on #609 if you'd prefer to see this one green before review.

Related: #608 (integration workflow), #609 (dev-group pin).

… APIs
TestAsyncMode multi-agent/bidi callbacks: both tests read the private
registry._registered_callbacks, which now holds strands' _CallbackEntry
wrapper objects rather than raw functions, so iscoroutinefunction() was
always False. Switched to the public registry.get_callbacks_for(event),
matching the sibling async tests that already pass. This also makes the
bidi init 'not any(iscoroutinefunction(...))' assertion meaningful --
it previously passed only because the wrapper is never a coroutine fn.
EvaluatorOutput validator: the error message claimed 'Either label,
value, or errorCode must be set', but the validator only accepts label
or errorCode and never inspects value. Corrected the message to match
the documented and implemented behaviour.
test_evaluation_with_empty_trajectory: run_evaluations() returns a
single EvaluationReport, not a list, so reports[0] raised TypeError.
Dropped the index and fixed the same stale pattern in the README, which
would have handed users the same TypeError.
@github-actions

Copy link
Copy Markdown
Contributor

✅ No Breaking Changes Detected

No public API breaking changes found in this PR.

@agentcore-devx-automationagentcore-devx-automationBot added the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automationagentcore-devx-automationBot removed the claude-security-reviewing Claude Code /security-review in progress label Jul 30, 2026
@jariy17
jariy17 marked this pull request as ready for review July 31, 2026 18:46
@jariy17
jariy17 requested a review from a teamJuly 31, 2026 18:46
@jariy17
jariy17 merged commit 53b0b48 into mainJul 31, 2026
37 of 42 checks passed
@jariy17
jariy17 deleted the fix/unit-test-drift branch July 31, 2026 19:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/sPR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@jariy17@notgitika