[Jany26]

• also moved out imports within functions
• reworked test_zstream_search.py helper function

AFTER REVIEWS:

• documented preferring flexmock in CONTRIBUTING.md
• also unified (at least across these modules) naming of those small helper mock functions to _mock_*()

[opohorel]

there are some findings from the Codex:

The flexmock migration introduces two test-coverage regressions:

  1. test_fork_and_prepare_modular_rhel_skips_create_zstream_branch builds tool_names from only the first call’s positional arguments, resulting in ["f"]. It should iterate over all captured calls:

  tool_names = [args[0] for args, _ in calls]

  2. test_mixed_merged_and_open_mrs no longer verifies that commits are fetched specifically from the merged MR. Since _mock_fetch ignores its URL argument, selecting the open or wrong MR would still pass. Please preserve the previous URL
     and call-count assertion, for example using .with_args(merged_mr_url).once().

  Otherwise, the migration looks good and CI is green.

[nforro]

Could we also add a mention to CONTRIBUTING.md or somewhere that flexmock is the preferred mocking framework, so we don't have to repeat this in the future?

[opohorel]

LGTM, thanks for looking into this!

[nforro]

There seem to be some issues:

1. Medium: test_reproducer_lock.py:102-115
flexmock().should_receive("__call__") does not intercept calls. Both tests pass for the wrong reason, leaving the skip and fallback behavior unverified.

2. Medium: test_reproducer_agent.py:854
fake_workflow() accepts no arguments, but production supplies several. The resulting TypeError is caught and retried, so this happy-path test instead exercises failure handling.

3. Medium: test_mr_consolidation.py:348
   The .never() expectation is installed after execution and also targets an unsupported bare __call__. It cannot detect an erroneous completion call.

4. Low: test_triage_agent.py:622
   The replacement for synchronous get_mock_local_tool_env() is async. Production receives a truthy coroutine rather than None, changing the tested path and potentially emitting an unawaited-coroutine warning.

5. Low: test_tasks.py:265
mock_jira_comment() accepts no arguments. The test catches an argument-binding TypeError, not the intended Jira RuntimeError.

6. Low: test_main.py:485-508
   Replacing os.environ with {} makes the environment-preservation assertions vacuously pass, so existing mock Jira values are no longer tested.

[Jany26]

/agentic_review
edit: nevermind, I have to onboard to qodo first
edit 2: fixed all oversights from Nikola

[nforro]

/agentic_review

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Four build-agent tests crash ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

test_failure_agent_has_no_build_or_edit_tools passes a bare flexmock() to replace_with,
installing a non-callable object as the implementation of get_chat_model instead of configuring
its return value. Every parameterized case calls create_build_failure_agent, which evaluates
get_chat_model() while constructing ReasoningAgent and therefore fails before the tool-filtering
assertions can inspect the tool configuration.

Code

ymir/agents/tests/unit/test_build_agent.py[479]

+    flexmock(build_agent).should_receive("get_chat_model").replace_with(flexmock())

Relevance

●●● Strong

The bare replacement is non-callable and deterministically crashes every test case during agent
construction.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed test installs the bare mock as the implementation of get_chat_model and then
immediately calls create_build_failure_agent. The production factory unconditionally passes
llm=get_chat_model() to ReasoningAgent, proving that the replacement must either be callable or
be configured as the function's return value.

ymir/agents/tests/unit/test_build_agent.py[478-483]
ymir/agents/build_agent.py[191-194]
ymir/agents/build_agent.py[191-195]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test installs a bare Flexmock object as the callable replacement for `get_chat_model`, but `create_build_failure_agent` invokes that dependency, so execution fails before the test can inspect the agent's tool configuration.

## Fix Focus Areas
- ymir/agents/tests/unit/test_build_agent.py[479-479]

## Recommended Fix
Replace `.replace_with(flexmock())` with `.and_return(flexmock())`, or provide a callable replacement that returns the mocked model, so the factory can invoke `get_chat_model()` successfully.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. The Y-stream mapping test always crashes ✓ Resolved 🐞 Bug ≡ Correctness

Description

test_pending_dependencies_maps_to_postponed_y_stream passes a bare flexmock() as
triage_agent_factory, which does not configure the object as callable. run_workflow invokes that
value immediately after entering the MCP-tools context, so this regression test raises before it can
verify the pending-dependency resolution path.

Code

ymir/agents/tests/unit/test_triage_agent.py[629]

+        triage_agent_factory=flexmock(),

Relevance

●●● Strong

Bare flexmock is not callable, causing the regression test to fail before workflow assertions.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed test passes the bare mock as the factory, while the production workflow directly calls
the factory before building or executing its nodes. Therefore execution cannot reach the assertions
that follow the workflow call.

ymir/agents/tests/unit/test_triage_agent.py[626-633]
ymir/agents/triage_agent.py[542-549]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test_pending_dependencies_maps_to_postponed_y_stream` passes a bare `flexmock()` as `triage_agent_factory`, but `run_workflow` calls the supplied factory. The test consequently fails with a non-callable mock before reaching its resolution assertions.

## Fix Focus Areas
- ymir/agents/tests/unit/test_triage_agent.py[626-630]
- ymir/agents/triage_agent.py[546-547]

## Recommended Fix
Replace the bare `flexmock()` argument with a callable factory, such as a local function accepting the gateway tools and local options and returning a mock agent. This test's pending-dependency path does not need to invoke the returned agent, so the returned mock can remain otherwise unconfigured.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Reproducer queue tests fail to start ✓ Resolved 🐞 Bug ≡ Correctness

Description

_run_process_task passes the span_processor mock to replace_with, which installs it as a
callable replacement for setup_observability rather than as that function's return value. Each
process-task test invokes main(), which calls setup_observability(...) and then accesses
start_transaction on its result, so these tests fail during harness setup instead of exercising
duplicate, lock, configuration, or workflow handling.

Code

ymir/agents/tests/unit/test_reproducer_agent.py[767]

+    flexmock(r_agent).should_receive("setup_observability").replace_with(span_processor)

Relevance

●●● Strong

Concrete flexmock API misuse prevents every process-task test from starting.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The modified helper invokes main(), while production main() calls setup_observability and
immediately uses the returned object's start_transaction method. The analogous triage-agent test
correctly configures the same kind of mock return using and_return(span_processor), demonstrating
the required flexmock API usage.

ymir/agents/tests/unit/test_reproducer_agent.py[747-771]
ymir/agents/reproducer_agent.py[1151-1154]
ymir/agents/tests/unit/test_triage_agent.py[140-152]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
`setup_observability` is mocked with `replace_with(span_processor)`, but `replace_with` requires a callable replacement. The test needs this function to return the prepared `span_processor` mock.

Fix Focus Areas
- ymir/agents/tests/unit/test_reproducer_agent.py[767-767]

Recommended Fix
Replace `.replace_with(span_processor)` with `.and_return(span_processor)`, matching the existing queue-process test harness pattern in `test_triage_agent.py`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Repeated middleware calls pass tests ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The migrated test_raw_results_are_unchanged tracks middleware execution with a boolean instead of
retaining an exact invocation expectation. If run_tool calls middleware more than once, the
boolean remains true and this coverage no longer detects the duplicate execution.

Code

ymir/common/tests/unit/test_run_tool.py[R38-49]

+    async def _mock_middleware(*_args, **_kwargs):
+        nonlocal middleware_awaited
+        middleware_awaited = True
+        return output

-    assert await run_tool(tool, query="hello") == expected
+    _mock_tool = flexmock(name="example")
+    _mock_tool.should_receive("run").with_args(input={"query": "hello"}).and_return(
+        flexmock(middleware=_mock_middleware)
+    ).once()

-    tool.run.assert_called_once_with(input={"query": "hello"})
-    tool.run.return_value.middleware.assert_awaited_once()
+    assert await run_tool(_mock_tool, query="hello") == expected
+    assert middleware_awaited

Relevance

●●● Strong

The finding preserves an explicit regression gap; recent history accepts strengthening behavioral
regression tests.

PR-#785

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The replacement middleware only sets a boolean, and the assertion checks only that boolean.
run_tool invokes middleware as the single tool-execution step, so preserving an exact-count
assertion is what detects accidental duplicate execution.

ymir/common/tests/unit/test_run_tool.py[35-49]
ymir/common/utils.py[121-128]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
The test only records whether middleware ran at least once, so repeated middleware execution is accepted despite the previous exact-await assertion.

Fix Focus Areas
- ymir/common/tests/unit/test_run_tool.py[38-49]

Recommended Fix
Track the middleware call count, or configure a flexmock expectation on a middleware mock, and assert it is invoked exactly once in each migrated run-tool test.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Empty delayed queues can skip their lookup ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

test_promote_due_tasks_noop_when_empty configures zrangebyscore without requiring a call, so the
test passes even if promote_due_tasks returns 0 before consulting Redis. The production function
is meant to perform that lookup before its empty-queue return, and the previous test explicitly
asserted that it was awaited once.

Code

ymir/common/tests/unit/test_delayed_queue.py[89]

+    redis.should_receive("zrangebyscore").replace_with(_mock_zrangebyscore)

Relevance

●●● Strong

The relaxed expectation permits skipping the required Redis lookup and weakens regression coverage.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The production path awaits redis_conn.zrangebyscore(...) and only then returns zero when the
result is empty. The rewritten expectation permits zero calls, whereas the prior unittest mock
assertion required one await.

ymir/common/delayed_queue.py[44-47]
ymir/common/tests/unit/test_delayed_queue.py[84-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The empty delayed-queue test no longer verifies that it queries Redis before returning zero, allowing an early-return regression to pass.

## Fix Focus Areas
- ymir/common/tests/unit/test_delayed_queue.py[85-90]

## Recommended Fix
Add `.once()` to the `zrangebyscore` flexmock expectation (or retain an equivalent explicit call-count assertion) so the test requires exactly one lookup.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 8 rules

Review mode: 🧠 Deep: This push combines a broad mock migration with substantive production changes across agents, models, utilities, and privileged tooling, creating many independent logic sites where redundant review could catch subtle defects.

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 0828c29

Results up to commit 6446179 🧠 Deep

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Reproducer queue tests fail to start ✓ Resolved 🐞 Bug ≡ Correctness

Description

_run_process_task passes the span_processor mock to replace_with, which installs it as a
callable replacement for setup_observability rather than as that function's return value. Each
process-task test invokes main(), which calls setup_observability(...) and then accesses
start_transaction on its result, so these tests fail during harness setup instead of exercising
duplicate, lock, configuration, or workflow handling.

Code

ymir/agents/tests/unit/test_reproducer_agent.py[767]

+    flexmock(r_agent).should_receive("setup_observability").replace_with(span_processor)

Relevance

●●● Strong

Concrete flexmock API misuse prevents every process-task test from starting.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The modified helper invokes main(), while production main() calls setup_observability and
immediately uses the returned object's start_transaction method. The analogous triage-agent test
correctly configures the same kind of mock return using and_return(span_processor), demonstrating
the required flexmock API usage.

ymir/agents/tests/unit/test_reproducer_agent.py[747-771]
ymir/agents/reproducer_agent.py[1151-1154]
ymir/agents/tests/unit/test_triage_agent.py[140-152]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

Issue description
`setup_observability` is mocked with `replace_with(span_processor)`, but `replace_with` requires a callable replacement. The test needs this function to return the prepared `span_processor` mock.

Fix Focus Areas
- ymir/agents/tests/unit/test_reproducer_agent.py[767-767]

Recommended Fix
Replace `.replace_with(span_processor)` with `.and_return(span_processor)`, matching the existing queue-process test harness pattern in `test_triage_agent.py`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. The Y-stream mapping test always crashes ✓ Resolved 🐞 Bug ≡ Correctness

Description

test_pending_dependencies_maps_to_postponed_y_stream passes a bare flexmock() as
triage_agent_factory, which does not configure the object as callable. run_workflow invokes that
value immediately after entering the MCP-tools context, so this regression test raises before it can
verify the pending-dependency resolution path.

Code

ymir/agents/tests/unit/test_triage_agent.py[629]

+        triage_agent_factory=flexmock(),

Relevance

●●● Strong

Bare flexmock is not callable, causing the regression test to fail before workflow assertions.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed test passes the bare mock as the factory, while the production workflow directly calls
the factory before building or executing its nodes. Therefore execution cannot reach the assertions
that follow the workflow call.

ymir/agents/tests/unit/test_triage_agent.py[626-633]
ymir/agents/triage_agent.py[542-549]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test_pending_dependencies_maps_to_postponed_y_stream` passes a bare `flexmock()` as `triage_agent_factory`, but `run_workflow` calls the supplied factory. The test consequently fails with a non-callable mock before reaching its resolution assertions.

## Fix Focus Areas
- ymir/agents/tests/unit/test_triage_agent.py[626-630]
- ymir/agents/triage_agent.py[546-547]

## Recommended Fix
Replace the bare `flexmock()` argument with a callable factory, such as a local function accepting the gateway tools and local options and returning a mock agent. This test's pending-dependency path does not need to invoke the returned agent, so the returned mock can remain otherwise unconfigured.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Empty delayed queues can skip their lookup ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

test_promote_due_tasks_noop_when_empty configures zrangebyscore without requiring a call, so the
test passes even if promote_due_tasks returns 0 before consulting Redis. The production function
is meant to perform that lookup before its empty-queue return, and the previous test explicitly
asserted that it was awaited once.

Code

ymir/common/tests/unit/test_delayed_queue.py[89]

+    redis.should_receive("zrangebyscore").replace_with(_mock_zrangebyscore)

Relevance

●●● Strong

The relaxed expectation permits skipping the required Redis lookup and weakens regression coverage.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The production path awaits redis_conn.zrangebyscore(...) and only then returns zero when the
result is empty. The rewritten expectation permits zero calls, whereas the prior unittest mock
assertion required one await.

ymir/common/delayed_queue.py[44-47]
ymir/common/tests/unit/test_delayed_queue.py[84-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The empty delayed-queue test no longer verifies that it queries Redis before returning zero, allowing an early-return regression to pass.

## Fix Focus Areas
- ymir/common/tests/unit/test_delayed_queue.py[85-90]

## Recommended Fix
Add `.once()` to the `zrangebyscore` flexmock expectation (or retain an equivalent explicit call-count assertion) so the test requires exactly one lookup.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 6713f56

[Jany26]

When I rebased my branch after @nforro's approval, some new unittest.mock tests appeared after f0b2690 (Run Copr builds directly and use LLMs only for diagnosis), so I also modified those tests.

[Jany26]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit da1609a

[opohorel]

When I rebased my branch after @nforro's approval, some new unittest.mock tests appeared after f0b2690 (Run Copr builds directly and use LLMs only for diagnosis), so I also modified those tests.

Sorry about that. I should have checked it more carefully. Thanks for doing the work!

[Jany26]

@opohorel yeah no problem, it's just that this probably needs your another review :D at least the 3 new test modules: test_build_agent, test_mr_consolidation_build, test_run_tool

[opohorel]

IMO this is good to merge