Skip to content

fix(oocana): return Optional[str] for node_id instead of magic strings #460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leavesster wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(oocana): return Optional[str] for node_id instead of magic strings #460

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions oocana/oocana/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,12 +291,16 @@ def block_info(self) -> BlockDict:
return self.__block_info.block_dict()

@property
def node_id(self) -> str:
# fix: run block don't have node_id
def node_id(self) -> Optional[str]:
"""Get the node_id from the current execution stack.

Returns:
The node_id if available, or None when running in contexts
without a node_id (e.g., run_block).
"""
if len(self.__block_info.stacks) > 0:
return self.__block_info.stacks[-1].get("node_id", "unknown")
else:
return "none"
return self.__block_info.stacks[-1].get("node_id")
return None
Comment on lines +294 to +303
Copy link

Copilot AI Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from returning str to Optional[str] is a breaking API change. While the internal usage at line 386 has been properly updated to handle None, external callers of this property (such as executor/python_executor/block.py lines 134 and 146) are not updated in this PR and will break when node_id is None. These call sites use node_id directly in string formatting and file path construction, which will fail with None values.

Consider one of these approaches:

  1. Update all known call sites in the same PR to handle the Optional return type
  2. Add a deprecation period where the property returns "unknown" by default but logs a warning, giving consumers time to migrate
  3. Document this as a breaking change and coordinate updates across all dependent code

Copilot uses AI. Check for mistakes.

@property
def oomol_llm_env(self) -> OOMOL_LLM_ENV:
Expand Down Expand Up @@ -379,7 +383,7 @@ def __wrap_output_value(self, handle: str, value: Any):
from .serialization import compression_suffix, compression_options
suffix = compression_suffix(context=self)
compression = compression_options(context=self)
flow_node = self.__block_info.stacks[-1].get("flow", "unknown") + "-" + self.node_id
flow_node = self.__block_info.stacks[-1].get("flow", "unknown") + "-" + (self.node_id or "unknown")
serialize_path = f"{self.pkg_data_dir}/.cache/{string_hash(flow_node)}/{handle}{suffix}"
os.makedirs(os.path.dirname(serialize_path), exist_ok=True)
try:
Expand Down
Loading