-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(utils): Add support for nested state access in template injection #3673
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.
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
Jainish-S
wants to merge
1
commit into
google:main
Choose a base branch
from
Jainish-S:feat/575-nested-state-template
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+273
−20
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
|
|
||
| import logging | ||
| import re | ||
| from typing import Any | ||
|
|
||
| from ..agents.readonly_context import ReadonlyContext | ||
| from ..sessions.state import State | ||
|
|
@@ -46,7 +47,11 @@ async def build_instruction( | |
| ) -> str: | ||
| return await inject_session_state( | ||
| 'You can inject a state variable like {var_name} or an artifact ' | ||
| '{artifact.file_name} into the instruction template.', | ||
| '{artifact.file_name} into the instruction template.' | ||
| 'You can also inject a nested variable like {var_name.nested_var}.' | ||
| 'If a variable or nested attribute may be missing, append `?` to the ' | ||
| 'path or attribute name for optional handling, e.g. ' | ||
| '{var_name.optional_nested_var?}.', | ||
| readonly_context, | ||
| ) | ||
|
|
||
|
|
@@ -78,14 +83,52 @@ async def _async_sub(pattern, repl_async_fn, string) -> str: | |
| result.append(string[last_end:]) | ||
| return ''.join(result) | ||
|
|
||
| def _get_nested_value(obj: Any, path: str) -> Any: | ||
| """Retrieve nested value from an object based on dot-separated path.""" | ||
| parts = path.split('.') | ||
| current = obj | ||
|
|
||
| for part in parts: | ||
| if current is None: | ||
| return None | ||
|
|
||
| optional = part.endswith('?') | ||
| key = part[:-1] if optional else part | ||
|
|
||
| # Try dictionary access first | ||
| if hasattr(current, '__getitem__'): | ||
| try: | ||
| current = current[key] | ||
| continue | ||
| except (KeyError, TypeError): | ||
| # If dict access fails, fall through to try getattr | ||
| # UNLESS it's a pure dict which definitely doesn't have attributes | ||
| if isinstance(current, dict): | ||
| if optional: | ||
| return None | ||
| raise KeyError(f"Key '{key}' not found in path '{path}'") | ||
| pass | ||
|
|
||
| # Try attribute access | ||
| try: | ||
| current = getattr(current, key) | ||
| except AttributeError: | ||
| # Both dict access and attribute access failed. | ||
| if optional: | ||
| return None | ||
| raise KeyError(f"Key '{key}' not found in path '{path}'") | ||
|
|
||
| return current | ||
|
|
||
| async def _replace_match(match) -> str: | ||
| var_name = match.group().lstrip('{').rstrip('}').strip() | ||
| optional = False | ||
| if var_name.endswith('?'): | ||
| optional = True | ||
| var_name = var_name.removesuffix('?') | ||
| if var_name.startswith('artifact.'): | ||
| var_name = var_name.removeprefix('artifact.') | ||
| full_path = match.group().lstrip('{').rstrip('}').strip() | ||
|
|
||
| if full_path.startswith('artifact.'): | ||
| var_name = full_path.removeprefix('artifact.') | ||
| optional = var_name.endswith('?') | ||
| if optional: | ||
| var_name = var_name[:-1] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| if invocation_context.artifact_service is None: | ||
| raise ValueError('Artifact service is not initialized.') | ||
| artifact = await invocation_context.artifact_service.load_artifact( | ||
|
|
@@ -104,22 +147,17 @@ async def _replace_match(match) -> str: | |
| raise KeyError(f'Artifact {var_name} not found.') | ||
| return str(artifact) | ||
| else: | ||
| if not _is_valid_state_name(var_name): | ||
| if not _is_valid_state_name(full_path.split('.')[0].removesuffix('?')): | ||
| return match.group() | ||
| if var_name in invocation_context.session.state: | ||
| value = invocation_context.session.state[var_name] | ||
|
|
||
| try: | ||
| value = _get_nested_value(invocation_context.session.state, full_path) | ||
|
|
||
| if value is None: | ||
| return '' | ||
| return str(value) | ||
| else: | ||
| if optional: | ||
| logger.debug( | ||
| 'Context variable %s not found, replacing with empty string', | ||
| var_name, | ||
| ) | ||
| return '' | ||
| else: | ||
| raise KeyError(f'Context variable not found: `{var_name}`.') | ||
| except KeyError as e: | ||
| raise KeyError(f'Context variable not found: `{full_path}`.') from e | ||
|
|
||
| return await _async_sub(r'{+[^{}]*}+', _replace_match, template) | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve consistency across the file for stripping the optional marker, you could use
removesuffix('?'). This method is already used on line 150 and is generally safer and more readable than slicing.