Skip to content

Fix/skill template#1952

Open
chenjw wants to merge 1 commit intomainfrom
fix/skill_template
Open

Fix/skill template#1952
chenjw wants to merge 1 commit intomainfrom
fix/skill_template

Conversation

@chenjw
Copy link
Copy Markdown
Collaborator

@chenjw chenjw commented May 9, 2026

Description

修复skill的bug for vaka

Related Issue

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🏅 Score: 75
🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add link models and merge logic

Relevant files:

  • openviking/session/memory/dataclass.py
  • openviking/session/memory/merge_op/link_merge.py
  • openviking/session/memory/page_id_map.py
  • tests/unit/session/memory/test_link_merge.py
  • tests/unit/session/memory/test_link_models.py
  • tests/unit/session/memory/test_page_id_map.py

Sub-PR theme: Integrate link extraction into memory pipeline

Relevant files:

  • openviking/session/memory/extract_loop.py
  • openviking/session/memory/memory_updater.py
  • openviking/session/memory/schema_model_generator.py
  • openviking/session/memory/session_extract_context_provider.py
  • openviking/prompts/templates/memory/*.yaml

Sub-PR theme: Add Jaeger trace debugging script

Relevant files:

  • tests/debug_trace.py

⚡ Recommended focus areas for review

Links may not be attached to upsert operations correctly

_distribute_links_to_operations is called before supplement_operation_uris. Although extract_loop.resolve_operations already populates URIs, this order is fragile and could break if the flow changes. Move _distribute_links_to_operations after supplement_operation_uris to ensure URIs are always available.

# Distribute resolved_links to corresponding upsert operations
self._distribute_links_to_operations(operations)

# 为每个upsert operation填充需要更新的uri列表
supplement_operation_uris(
    operations,
    self._registry,
    extract_context=extract_context,
    isolation_handler=isolation_handler,
Unhandled ValueError when exceeding 99 existing pages

register_existing raises ValueError when more than 99 existing pages are registered, but callers in extract_loop do not handle this exception. This could crash the extract loop in production. Add error handling or relax the limit with a fallback strategy.

def register_existing(self, uri: str) -> int:
    """Register an existing page (from prefetch/read). Returns page_id in 1-99 range."""
    if uri in self._uri_to_id:
        return self._uri_to_id[uri]
    if self._next_id > self.MAX_EXISTING_ID:
        raise ValueError(f"Too many existing pages for PageIdMap (max {self.MAX_EXISTING_ID})")
    page_id = self._next_id
    self._next_id += 1
    self._id_to_uri[page_id] = uri
    self._uri_to_id[uri] = page_id
    return page_id
Template rendering now re-raises exceptions (breaking change)

Previously, template rendering errors were swallowed and the function continued. Now it logs and re-raises. This could crash existing flows if callers do not handle the exception. Consider making this configurable or ensuring callers handle failures gracefully.

except Exception as e:
    from openviking.telemetry import tracer

    tracer.error(f"Template rendering failed, skipping write: {e}")
    raise

- skills.yaml: use |default(0, true) to handle None values in Jinja2
  template (default() only replaces undefined, not None)
- content.py: log and raise on template rendering failure instead of
  silently swallowing the exception (results in empty content)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add support for registering new pages with specific page IDs

Add a method to register a URI with a specific page ID (for new pages with
LLM-assigned IDs >=100). This fixes the bug where op.page_id was ignored during
registration, breaking link resolution.

openviking/session/memory/page_id_map.py [39-47]

-def register_new(self, uri: str) -> int:
-    """Register a new page (from LLM output). Returns page_id >= 100."""
+def register_new(self, uri: str, page_id: Optional[int] = None) -> int:
+    """Register a new page (from LLM output). Returns page_id >= 100.
+    
+    Args:
+        uri: URI to register
+        page_id: Optional specific page ID to use (must be >=100 if provided)
+    """
     if uri in self._uri_to_id:
         return self._uri_to_id[uri]
-    page_id = self._next_new_id
-    self._next_new_id += 1
+    
+    if page_id is not None:
+        if page_id < 100:
+            raise ValueError(f"New page IDs must be >=100, got {page_id}")
+        if page_id in self._id_to_uri:
+            raise ValueError(f"Page ID {page_id} already registered for URI {self._id_to_uri[page_id]}")
+    else:
+        page_id = self._next_new_id
+    
     self._id_to_uri[page_id] = uri
     self._uri_to_id[uri] = page_id
+    
+    if page_id >= self._next_new_id:
+        self._next_new_id = page_id + 1
+    
     return page_id
Suggestion importance[1-10]: 8

__

Why: This fixes a critical bug where LLM-assigned page IDs were ignored during registration, breaking link resolution for new pages. The change allows explicit page ID assignment for new pages.

Medium
Use LLM-assigned page IDs when registering new pages

Pass the op.page_id to register_new when registering new pages. This ensures the
LLM-assigned page ID is used, allowing link resolution to work correctly.

openviking/session/memory/extract_loop.py [346-350]

 # Register new page_ids (100+) after URI resolution
 for op in upsert_operations:
     if op.page_id is not None and op.page_id >= 100:
         for uri in op.uris:
-            self._page_id_map.register_new(uri)
+            self._page_id_map.register_new(uri, page_id=op.page_id)
Suggestion importance[1-10]: 8

__

Why: This addresses the core issue where op.page_id was not passed to register_new, ensuring LLM-assigned page IDs are correctly mapped to URIs for link resolution.

Medium

@chenjw chenjw force-pushed the fix/skill_template branch from 6025d52 to 8bd4a29 Compare May 9, 2026 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant