Fix/skill template

[chenjw]

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]

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

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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