[utsab345]

Summary

• inspect Pinecone upsert responses for batch-level errors
• log the failed item count and raise before reporting the index update as done
• add regression coverage for object and dict-shaped error responses

Closes #3544

Tests

• PYTHONPATH=. uv run pytest tests/unittest/test_similar_issue_pinecone.py -q --color=no
• uv run ruff check --fix pr_agent/tools/pr_similar_issue.py tests/unittest/test_similar_issue_pinecone.py
• uv run pre-commit run --files pr_agent/tools/pr_similar_issue.py tests/unittest/test_similar_issue_pinecone.py

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fail similar-issue indexing on Pinecone batch errors

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Detect Pinecone batch failures returned after similar-issue vector upserts.
• Log failed vector counts and stop before marking failed updates complete.
• Cover object- and dictionary-shaped error responses with regression tests.

Diagram

graph TD
  A["Issue indexing"] --> B["Pinecone upsert"] --> C{"Batch errors?"}
  C -- "Yes" --> D["Log failed count"] --> E["Raise RuntimeError"]
  C -- "No" --> F["Finalize update"]

Loading

High-Level Assessment

The helper-based response validation is appropriate because Pinecone can return failures without raising and may expose responses as objects, dictionaries, or convertible models. Inline validation would duplicate response-normalization concerns, while relying only on SDK exceptions would preserve the original silent-failure behavior.

Files changed (2) +64 / -4

Bug fix (1) +25 / -4

pr_similar_issue.pyValidate Pinecone upsert responses before completing indexing +25/-4

Validate Pinecone upsert responses before completing indexing

• Adds a response validator supporting dictionary, attribute-based, and 'to_dict()' response shapes. Pinecone batch failures now log the failed item count and raise before the finalization delay or completion message.

pr_agent/tools/pr_similar_issue.py

Tests (1) +39 / -0

test_similar_issue_pinecone.pyCover Pinecone object and dictionary error responses +39/-0

Cover Pinecone object and dictionary error responses

• Adds regression tests confirming both response shapes raise with the failed vector count. The tests also verify errors are logged and prevent the post-upsert sleep.

tests/unittest/test_similar_issue_pinecone.py

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

1. Failed vectors can be reported indexed ✓ Resolved 📎 Requirement gap ≡ Correctness

Description

_raise_on_pinecone_upsert_errors returns whenever has_errors is false or absent, before it
evaluates failed_item_count. A Pinecone response with a positive failure count but no true flag
therefore proceeds through readiness polling and the final Done log.

Code

pr_agent/tools/pr_similar_issue.py[R80-82]

+    has_errors = getattr(response, "has_errors", response_dict.get("has_errors", False))
+    if not has_errors:
+        return

Evidence

Compliance rule 3175559 explicitly prohibits treating a nonzero failed item count as success. The
added handler returns based only on has_errors, while the update path subsequently performs
readiness polling and logs completion.

Handle Pinecone Batched Upsert Failures
pr_agent/tools/pr_similar_issue.py[80-89]
pr_agent/tools/pr_similar_issue.py[607-609]

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 Pinecone response handler treats `has_errors` as the sole failure signal, so a nonzero `failed_item_count` can be ignored when the flag is false or absent.
## Fix Focus Areas
- pr_agent/tools/pr_similar_issue.py[80-89]
- tests/unittest/test_similar_issue_pinecone.py[145-154]
## Recommended Fix
Read `failed_item_count` before returning and raise when either `has_errors` is true or the count is nonzero. Add object-shaped and dictionary-shaped regression cases where the count is positive while `has_errors` is false or absent.

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

---

2. Index queries can race pending writes ✓ Resolved 📎 Requirement gap ☼ Reliability

Description

_update_index_with_issues passes upsert_response only to _raise_on_pinecone_upsert_errors,
which reads error fields but never uses the response LSN. When successful batches take longer than
five seconds to become query-ready, execution reaches the fixed sleep and announces Done without
definitive readiness.

Code

pr_agent/tools/pr_similar_issue.py[557]

+        _raise_on_pinecone_upsert_errors(upsert_response)

Evidence

Compliance rule 3175561 requires readiness to be verified using the LSN returned by Pinecone instead
of relying solely on a fixed delay. The changed call passes the response only to an error checker,
while the following code still uses time.sleep(5) and then reports completion.

Use Pinecone readiness information instead of a fixed delay
pr_agent/tools/pr_similar_issue.py[78-87]
pr_agent/tools/pr_similar_issue.py[553-559]

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 Pinecone upsert response is checked for batch errors, but its LSN is not used to establish write readiness before the update reports completion.
## Fix Focus Areas
- pr_agent/tools/pr_similar_issue.py[553-558]
## Recommended Fix
Extract the LSN from object- and dictionary-shaped successful upsert responses, then use Pinecone's readiness mechanism to wait until that LSN is query-ready with a bounded timeout. Remove the fixed five-second sleep and log `Done` only after readiness is confirmed.

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

---

3. Readiness timing cannot be configured 📘 Rule violation ⚙ Maintainability

Description

PINECONE_UPSERT_READY_TIMEOUT_SECONDS and PINECONE_UPSERT_READY_POLL_SECONDS hard-code
operational timing thresholds in the source module. Every installation is consequently forced to use
a 30-second deadline and half-second polling interval regardless of its Pinecone latency or request
budget.

Code

pr_agent/tools/pr_similar_issue.py[R16-17]

+PINECONE_UPSERT_READY_TIMEOUT_SECONDS = 30
+PINECONE_UPSERT_READY_POLL_SECONDS = 0.5

Evidence

Compliance rule 2694652 requires deployment-sensitive thresholds to come from .pr_agent.toml or
pr_agent/settings. The added constants are directly consumed by the readiness loop and control its
timeout and polling behavior.

Rule 2694652: Do not hard-code configuration; load it from .pr_agent.toml or pr_agent/settings
pr_agent/tools/pr_similar_issue.py[16-17]
pr_agent/tools/pr_similar_issue.py[128-137]

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 new Pinecone timeout and polling thresholds determine runtime behavior but are embedded directly in business logic instead of being configurable through project settings.
## Fix Focus Areas
- pr_agent/tools/pr_similar_issue.py[16-17]
- pr_agent/tools/pr_similar_issue.py[128-137]
## Recommended Fix
Define documented defaults for both readiness values in `pr_agent/settings/configuration.toml`, read them through `get_settings()`, and use those configured values when calculating the deadline and polling delay.

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

---

4. Readiness checks can exceed 30 seconds 🐞 Bug ☼ Reliability

Description

_wait_for_pinecone_upsert_readiness performs each blocking fetch before checking whether its
deadline has expired. After the final sleep it always starts another request, and a slow or hung
request can extend the indexing operation well beyond the configured 30-second limit.

Code

pr_agent/tools/pr_similar_issue.py[R128-130]

+    deadline = time.monotonic() + PINECONE_UPSERT_READY_TIMEOUT_SECONDS
+    while True:
+        fetch_response = pinecone_index.fetch(ids=[vector_id], namespace=namespace)

Evidence

The deadline is established at line 128, but the unconditional network request at line 130 precedes
the first expiration check at lines 134-136. Because the loop sleeps for all remaining time at line
137 and then returns to line 130, it can begin an additional fetch after the deadline has already
elapsed.

pr_agent/tools/pr_similar_issue.py[128-137]
tests/unittest/test_similar_issue_pinecone.py[157-185]

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 Pinecone readiness loop checks its deadline only after a blocking fetch, so it can issue another request after the timeout and cannot bound time spent inside that request.
## Fix Focus Areas
- pr_agent/tools/pr_similar_issue.py[128-137]
## Recommended Fix
Check the remaining deadline before every fetch and execute each fetch with a supported request timeout no greater than the remaining readiness budget. Add coverage proving that no fetch starts after the deadline and that a slow request cannot overrun the configured timeout.

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

---

• Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Tip of the day

💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 4c240b9

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 5025ee2

[IsmaelMartinez]

Thanks for the same-morning turnaround on #3544 and for folding both Qodo findings in. This holds on the real pinecone 10.0.0 client: a failed batch now raises with the count logged, and the LSN poll replaces the fixed sleep.

One ask inline: the SDK keeps each failed batch's error message, so log it beside the count, or the operator sees "100 vectors" and no cause.

#3560 landed first this afternoon, so this needs a rebase: the repository namespace now goes on your upsert and on the readiness fetch.

[IsmaelMartinez]

The SDK keeps each failed batch's error_message; without it this log says how many vectors failed and nothing about why.

Suggested change

	get_logger().error(
	"Pinecone upsert failed",
	artifact={"failed_item_count": failed_item_count},
	)
	errors = getattr(response, "errors", response_dict.get("errors")) or []
	get_logger().error(
	"Pinecone upsert failed",
	artifact={"failed_item_count": failed_item_count,
	"errors": [_get_value(error, "error_message") for error in errors]},
	)

[IsmaelMartinez]

Matching expectation for the artifact above.

Suggested change

	assert logged == [{"failed_item_count": 100}]
	assert logged == [{"failed_item_count": 100, "errors": []}]

[qodo-free-for-open-source-projects]

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

[IsmaelMartinez]

Thanks for extending this to a full readiness check and folding in both Qodo findings, this needs one change before merge. The core fix holds: reverting your source while keeping your tests turns 6 of 20 green tests red, and the LSN wait terminates cleanly under both fast and stuck reconciliation. One inline ask: the wait loop calls fetch before checking its deadline, so a slow fetch can push the wait past the configured 30 seconds; suggestion attached, applied to your head and tests still pass. Full suite is green (9184 passed, no regressions), cleanly rebased on #3560's namespace change.

[IsmaelMartinez]

The wait loop must check its deadline before calling fetch, not after; a slow fetch can otherwise push the total wait past the configured 30 seconds.

Suggested change

	while True:
	fetch_response = pinecone_index.fetch(ids=[vector_id], namespace=namespace)
	while True:
	if time.monotonic() >= deadline:
	raise TimeoutError(f"Pinecone upsert was not query-ready after {PINECONE_UPSERT_READY_TIMEOUT_SECONDS}s")
	fetch_response = pinecone_index.fetch(ids=[vector_id], namespace=namespace)

[utsab345]

Applied on the head commit a1c587a: the loop now raises when time.monotonic() >= deadline before calling fetch, so a slow fetch can no longer stretch the wait past the configured timeout. Added a regression test that pins the loop stops polling once the deadline is passed.

[qodo-free-for-open-source-projects]

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

[IsmaelMartinez]

Thanks for the quick fix, this is ready to merge. The wait loop now checks its deadline before calling fetch, so a slow request can no longer push past the 30 second budget: reverting your source while keeping the tests turns 7 of 14 red, and a timed harness with a slow fake fetch cuts worst case overshoot from about 97% to under 4%. Full suite is green at 9193 passed, exactly baseline plus your tests, and CI is all green.