Skip to content

fix(memory): require parseable final extract output#1902

Merged
chenjw merged 2 commits intovolcengine:mainfrom
duyua9:fix/extract-loop-final-json-duyua9
May 8, 2026
Merged

fix(memory): require parseable final extract output#1902
chenjw merged 2 commits intovolcengine:mainfrom
duyua9:fix/extract-loop-final-json-duyua9

Conversation

@duyua9
Copy link
Copy Markdown
Contributor

@duyua9 duyua9 commented May 8, 2026

Summary

  • make the final ExtractLoop iteration prompt JSON-only and schema-aware with an explicit empty operations skeleton
  • fail the final unparseable response path instead of silently returning empty ResolvedOperations
  • add focused regression coverage for the final prompt shape and final unparseable response behavior

Validation

  • python3 -m py_compile openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py
  • git diff --check
  • uvx ruff check openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py
  • uvx ruff format openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py

Targeted pytest note: this local checkout cannot import the target memory test module with system Python because optional project dependency litellm is not installed; uv run python -m pytest ... also cannot complete here because the editable build tries to build the Rust CLI and the local macOS CommandLineTools xcrun is an incompatible architecture. The new tests are still included for CI.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🏅 Score: 70
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Critical AttributeError

self._expected_fields is initialized to None but append() is called on it without first initializing to a list, causing AttributeError on first run.

self._expected_fields.append(f"{schema.memory_type}")
Test NameError

The 'result' variable is referenced outside the with block where it is defined, causing a NameError.

assert "viking://agent/default/memories/tools" in result
Missing delete_uris in skeleton

The final operations skeleton does not include delete_uris unless _expected_fields is None, which may cause model outputs to omit a required field.

return {field: [] for field in self._expected_fields or ["delete_uris"]}

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix uninitialized _expected_fields list

Initialize self._expected_fields to an empty list before appending to it. Without
initialization, self._expected_fields remains None and calling .append() will raise
an AttributeError.

openviking/session/memory/extract_loop.py [135-140]

 # 获取 ExtractContext(整个流程复用)
 self._extract_context = self.context_provider.get_extract_context()
 if self._extract_context is None:
     raise ValueError("Failed to get ExtractContext from provider")
+self._expected_fields = []
 for schema in schemas:
     self._expected_fields.append(f"{schema.memory_type}")
Suggestion importance[1-10]: 8

__

Why: The suggestion addresses a critical bug where self._expected_fields is used as a list without being initialized (it was set to None), which would cause an AttributeError when calling .append(). The fix is accurate and necessary for the code to function correctly.

Medium

@qin-ctx qin-ctx requested a review from chenjw May 8, 2026 03:20
@duyua9
Copy link
Copy Markdown
Contributor Author

duyua9 commented May 8, 2026

Pushed a2ac581e to address the automated review feedback:

  • Kept delete_uris in the final empty-operations skeleton even if _expected_fields is missing or does not include it, and added a regression for that path.
  • Moved the existing allowed-directory assertion back into the same patch scope to avoid any ambiguity in the test.
  • Confirmed _expected_fields is initialized before appending memory type fields in run().

Validation:

  • uvx ruff check openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py
  • uvx ruff format --check openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py
  • python3 -m py_compile openviking/session/memory/extract_loop.py tests/session/memory/test_memory_react.py
  • git diff --check
  • identity gate for duyua9 passed before push

@chenjw
Copy link
Copy Markdown
Collaborator

chenjw commented May 8, 2026

Looks good to me, thanks.

@chenjw chenjw merged commit 373665f into volcengine:main May 8, 2026
5 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in OpenViking project May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants