-
Notifications
You must be signed in to change notification settings - Fork 60
feat(cli): add runtime.log_file YAML default for --log-file #473
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
base: main
Are you sure you want to change the base?
Changes from all commits
4b98de5
8142eb1
78e328e
df91d12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -139,6 +139,22 @@ def init_file_logging(log_path: Path) -> None: | |
| ) | ||
|
|
||
|
|
||
| def _try_init_file_logging(log_path: Path | None) -> bool: | ||
| """Initialize file logging, returning whether initialization succeeded.""" | ||
| if log_path is None: | ||
| return False | ||
|
|
||
| try: | ||
| init_file_logging(log_path) | ||
| except OSError as e: | ||
| _verbose_console.print( | ||
| styled("[bold yellow]Warning:[/bold yellow] Cannot open log file {}: {}", log_path, e) | ||
| ) | ||
| return False | ||
|
|
||
| return True | ||
|
|
||
|
|
||
| def close_file_logging() -> None: | ||
| """Close file logging and clean up resources.""" | ||
| global _file_console, _file_handle | ||
|
|
@@ -1959,15 +1975,7 @@ async def run_workflow_async( | |
| start_time = time.time() | ||
|
|
||
| # Initialize file logging if requested | ||
| if log_file is not None: | ||
| try: | ||
| init_file_logging(log_file) | ||
| except OSError as e: | ||
| _verbose_console.print( | ||
| styled( | ||
| "[bold yellow]Warning:[/bold yellow] Cannot open log file {}: {}", log_file, e | ||
| ) | ||
| ) | ||
| _try_init_file_logging(log_file) | ||
|
|
||
| # Always create event emitter and JSONL log subscriber | ||
| emitter = WorkflowEventEmitter() | ||
|
|
@@ -1995,6 +2003,15 @@ async def run_workflow_async( | |
| config = load_config(workflow_path) | ||
| verbose_log_timing("Configuration loaded", time.time() - load_start) | ||
|
|
||
| if log_file is None: | ||
| configured = config.workflow.runtime.log_file | ||
| if configured is not None: | ||
| if configured.lower() == "auto": | ||
| log_file = generate_log_path(workflow_path.stem) | ||
| else: | ||
| log_file = Path(configured) | ||
| _try_init_file_logging(log_file) | ||
|
|
||
| # Merge CLI metadata on top of YAML-declared metadata | ||
| if metadata: | ||
| config.workflow.metadata.update(metadata) | ||
|
|
@@ -2592,15 +2609,7 @@ async def resume_workflow_async( | |
| start_time = time.time() | ||
|
|
||
| # Initialize file logging if requested | ||
| if log_file is not None: | ||
| try: | ||
| init_file_logging(log_file) | ||
| except OSError as e: | ||
| _verbose_console.print( | ||
| styled( | ||
| "[bold yellow]Warning:[/bold yellow] Cannot open log file {}: {}", log_file, e | ||
| ) | ||
| ) | ||
| _try_init_file_logging(log_file) | ||
|
|
||
| # Always create event emitter and JSONL log subscriber (parity with run) | ||
| emitter = WorkflowEventEmitter() | ||
|
|
@@ -2659,6 +2668,15 @@ async def resume_workflow_async( | |
| # (workflow_root) and to seed the synthetic replay fallback. | ||
| config = load_config(resolved_workflow_path) | ||
|
|
||
| if log_file is None: | ||
| configured = config.workflow.runtime.log_file | ||
| if configured is not None: | ||
| if configured.lower() == "auto": | ||
| log_file = generate_log_path(resolved_workflow_path.stem) | ||
| else: | ||
| log_file = Path(configured) | ||
| _try_init_file_logging(log_file) | ||
|
Comment on lines
+2674
to
+2678
Collaborator
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. Same four issues as the run side. Given these two blocks are identical apart from the path variable, and AGENTS.md names drift between A shared One more thing specific to resume. Because the YAML value is read after the checkpoint block, this log misses |
||
|
|
||
| # Merge CLI metadata on top of YAML-declared metadata (parity with run) | ||
| if metadata: | ||
| config.workflow.metadata.update(metadata) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3332,6 +3332,9 @@ def _coerce_provider(cls, value: Any) -> Any: | |||||||||||||||
| default_model: str | None = None | ||||||||||||||||
| """Default model for agents that don't specify one.""" | ||||||||||||||||
|
|
||||||||||||||||
| log_file: str | None = None | ||||||||||||||||
| """Default log file path, or ``auto`` to generate one automatically.""" | ||||||||||||||||
|
Comment on lines
+3335
to
+3336
Collaborator
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. A couple of invariants the type could carry instead of the call site. An empty or whitespace-only value clears the Stripping also fixes The docstring is worth two additions as well:
Suggested change
|
||||||||||||||||
|
|
||||||||||||||||
| mcp_servers: dict[str, MCPServerDef] = Field(default_factory=dict) | ||||||||||||||||
| """MCP server configurations keyed by server name.""" | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
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.
Four things land in this block.
generate_log_path()is outside the guard. Itsmkdir(parents=True, exist_ok=True)still raisesFileExistsErrorwhen$TMPDIR/conductoris a file, andPermissionErrororENOSPCon a constrained runner. A logging setting shouldn't be able to abort the run, and it's inconsistent with the explicit-path branch one line below, which warns and continues.Path(configured)doesn't expand~. Sinceinit_file_loggingcreates parents,log_file: ~/logs/run.logquietly writes into a directory named~under the CWD and reports success. The shell saves the CLI flag from this; YAML doesn't.A NUL byte in the value raises
ValueError, whichexcept OSErrordoesn't catch, so it escapes as an unhandled traceback.And the
boolfrom_try_init_file_loggingis dropped here, solog_filekeeps naming a file that was never opened. Nothing reads it wrongly today, but only because the_file_console is not Nonecheck at line 2313 happens to cover for it.