fix: address human review feedback on PR #166

- Remove redundant isinstance checks in _resolve_model_config; Pydantic
  ModelConfigDocument validates dict[str, dict[str, Any]] at parse time,
  making runtime type checks dead code. Update test to verify Pydantic
  validation instead of testing the removed checks.

- Remove shadowing local imports of os and typer in cli.py that duplicate
  top-level imports.

- Don't inject default temperature (0.0) when api_type is 'responses';
  the responses API rejects unsupported parameters. MODEL_TEMP env var
  and YAML model_settings still override for both API types. Preserves
  backward-compatible 0.0 default for chat_completions.

- Narrow task-level retry to transient network exceptions only
  (APIConnectionError, APITimeoutError, ConnectionError, TimeoutError).
  Non-retriable errors (ValueError, RuntimeError, etc.) now break
  immediately instead of burning through retry attempts. Prevents blind
  retries of deterministic failures and side-effectful repeat_prompt
  re-execution on non-transient errors.

Author: anticomputer

src/seclab_taskflow_agent/cli.py

@@ -43,7 +43,6 @@ def _parse_global(value: str) -> tuple[str, str]:
 
 def _setup_logging() -> None:
     """Configure root logger: file (DEBUG) + console (ERROR)."""
-    import os
     from logging.handlers import RotatingFileHandler
 
     root = logging.getLogger("")
@@ -68,8 +67,6 @@ def _print_concise_error(exc: BaseException) -> None:
     Walks the exception cause chain and prints each error on a single
     line.  Use ``--debug`` or ``TASK_AGENT_DEBUG=1`` for full tracebacks.
     """
-    import typer
-
     seen: set[int] = set()
     current: BaseException | None = exc
     while current and id(current) not in seen:

src/seclab_taskflow_agent/runner.py

@@ -31,7 +31,7 @@
 from agents.agent import ModelSettings
 from agents.exceptions import AgentsException, MaxTurnsExceeded
 from agents.extensions.handoff_prompt import prompt_with_handoff_instructions
-from openai import APITimeoutError, BadRequestError, RateLimitError
+from openai import APIConnectionError, APITimeoutError, BadRequestError, RateLimitError
 from openai.types.responses import ResponseTextDeltaEvent
 
 from .agent import DEFAULT_MODEL, TaskAgent, TaskAgentHooks, TaskRunHooks
@@ -74,20 +74,13 @@ def _resolve_model_config(
     """
     m_config: ModelConfigDocument = available_tools.get_model_config(model_config_ref)
     model_dict: dict[str, str] = m_config.models or {}
-    if model_dict and not isinstance(model_dict, dict):
-        raise ValueError(f"Models section of the model_config file {model_config_ref} must be a dictionary")
     model_keys: list[str] = list(model_dict.keys())
     models_params: dict[str, dict[str, Any]] = m_config.model_settings or {}
-    if models_params and not isinstance(models_params, dict):
-        raise ValueError(f"Settings section of model_config file {model_config_ref} must be a dictionary")
     unknown = set(models_params) - set(model_keys)
     if unknown:
         raise ValueError(
             f"Settings section of model_config file {model_config_ref} contains models not in the model section: {unknown}"
         )
-    for k, v in models_params.items():
-        if not isinstance(v, dict):
-            raise ValueError(f"Settings for model {k} in model_config file {model_config_ref} is not a dictionary")
     return model_keys, model_dict, models_params, m_config.api_type
 
 
@@ -299,10 +292,17 @@ async def deploy_task_agents(
     # Model settings
     parallel_tool_calls = bool(os.getenv("MODEL_PARALLEL_TOOL_CALLS"))
     model_params: dict[str, Any] = {
-        "temperature": os.getenv("MODEL_TEMP", default=0.0),
         "tool_choice": "auto" if toolboxes else None,
         "parallel_tool_calls": parallel_tool_calls if toolboxes else None,
     }
+    # Only inject a default temperature for chat_completions; the responses
+    # API rejects unsupported parameters.  MODEL_TEMP env override applies
+    # to both API types.
+    model_temp = os.getenv("MODEL_TEMP")
+    if model_temp is not None:
+        model_params["temperature"] = model_temp
+    elif api_type != "responses":
+        model_params["temperature"] = 0.0
     model_params.update(model_par)
     model_settings = ModelSettings(**model_params)
 
@@ -660,7 +660,11 @@ async def _deploy(ra: dict, pp: str) -> bool:
                         complete = result and complete
                     return complete
 
-                # Execute the task with auto-retry on failure
+                # Execute the task with auto-retry on transient failures.
+                # Only retry on network/API errors — deterministic failures
+                # and errors after side-effectful work should not be retried
+                # blindly (e.g. repeat_prompt tasks may have already written
+                # data to external systems).
                 task_name = task.name or f"task-{task_index}"
                 task_complete = False
                 last_task_error: BaseException | None = None
@@ -675,7 +679,7 @@ async def _deploy(ra: dict, pp: str) -> bool:
                         break
                     except (KeyboardInterrupt, SystemExit):
                         raise
-                    except Exception as exc:
+                    except (APIConnectionError, APITimeoutError, ConnectionError, TimeoutError) as exc:
                         last_task_error = exc
                         remaining = TASK_RETRY_LIMIT - attempt - 1
                         if remaining > 0:
@@ -688,6 +692,10 @@ async def _deploy(ra: dict, pp: str) -> bool:
                             await asyncio.sleep(backoff)
                         else:
                             logging.error(f"Task {task_name!r} failed after {TASK_RETRY_LIMIT} attempts: {exc}")
+                    except Exception as exc:
+                        last_task_error = exc
+                        logging.error(f"Task {task_name!r} failed (non-retriable): {exc}")
+                        break
 
                 # If all retries exhausted with an exception, save and re-raise
                 if last_task_error is not None:

tests/test_runner.py

@@ -107,14 +107,10 @@ def test_model_settings_extraction(self):
         assert params == {"m1": {"temperature": 0.5}}
 
     def test_validation_error_on_non_dict_settings(self):
-        """Non-dict model_settings raises ValueError."""
-        at = _mock_available_tools()
-        cfg = _make_model_config(models={"m1": "p-m1"})
-        # Manually set model_settings to a non-dict after construction
-        object.__setattr__(cfg, "model_settings", "not-a-dict")
-        at.get_model_config.return_value = cfg
-        with pytest.raises(ValueError, match="must be a dictionary"):
-            _resolve_model_config(at, "ref")
+        """Pydantic rejects non-dict model_settings at parse time."""
+        from pydantic import ValidationError
+        with pytest.raises(ValidationError):
+            _make_model_config(models={"m1": "p-m1"}, model_settings="not-a-dict")
 
 
 # ===================================================================