fix: surface silent YAML/timeout collapses and unblock nested skill paths#3176
fix: surface silent YAML/timeout collapses and unblock nested skill paths#3176praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…aths (fixes #3175) - _normalize_yaml_config: detect duplicate agent/task names and unknown task agent refs; raise under PRAISONAI_VALIDATE_STRICT or warn loudly and preserve both entries by suffixing, instead of silently dropping. - _resolve_effective_tool_timeout: apply the tightest (min) declared tool_timeout to the shared tool dict and warn on any overridden agent, so a fast agent is never silently forced to wait a slower agent's budget. - BasicSkillMutator._is_safe_path: allow nested skill files (references/, scripts/, assets/) while still rejecting absolute paths and parent traversal; real containment stays enforced by the caller's resolve(). Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughYAML normalization now preserves duplicate entries and validates task references. Tool timeout resolution selects the tightest configured value. Skill path filtering rejects unsafe syntax while allowing valid nested paths. ChangesConfiguration safety
Skill path safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant AgentsGenerator
participant RoleAgentConfig
participant SharedToolDict
CLI->>AgentsGenerator: provide optional tool_timeout
AgentsGenerator->>RoleAgentConfig: read declared role and agent timeouts
RoleAgentConfig-->>AgentsGenerator: return timeout values
AgentsGenerator->>SharedToolDict: apply CLI timeout or tightest declared timeout
AgentsGenerator-->>CLI: emit override warnings when values differ
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/praisonai/praisonai/agents_generator.py`:
- Line 814: Update the entities construction in the agent configuration flow to
use an empty-dictionary fallback when either config.get("roles", {}) or
config.get("agents", {}) returns None. Preserve the existing merge behavior for
populated mappings while preventing dictionary unpacking of null values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46573732-e55d-46ba-b6dc-28dbab82eea1
📒 Files selected for processing (3)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/tools/skill_manage.pysrc/praisonai/tests/unit/test_tool_timeout_enforcement.py
| if isinstance(cli_timeout, (int, float)) and not isinstance(cli_timeout, bool): | ||
| return float(cli_timeout) | ||
|
|
||
| entities = {**config.get("roles", {}), **config.get("agents", {})} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent TypeError when configuration keys are explicit nulls.
If roles or agents are explicitly defined as empty in the YAML (e.g., roles: null), config.get("roles", {}) returns None. Using the dictionary unpacking operator (**) on None raises a TypeError.
Consider adding an or {} fallback to safely handle None values.
🛠️ Proposed fix
- entities = {**config.get("roles", {}), **config.get("agents", {})}
+ entities = {**(config.get("roles") or {}), **(config.get("agents") or {})}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| entities = {**config.get("roles", {}), **config.get("agents", {})} | |
| entities = {**(config.get("roles") or {}), **(config.get("agents") or {})} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/praisonai/praisonai/agents_generator.py` at line 814, Update the entities
construction in the agent configuration flow to use an empty-dictionary fallback
when either config.get("roles", {}) or config.get("agents", {}) returns None.
Preserve the existing merge behavior for populated mappings while preventing
dictionary unpacking of null values.
Greptile SummaryThis PR addresses three silent failure modes in the wrapper layer: duplicate YAML entries no longer collapse silently (with
Confidence Score: 4/5Safe to merge only if the Two of the three fixes change observable runtime behaviour in ways that can break valid configurations: agents with larger declared timeouts will now always time out as early as the smallest peer, and agents with duplicate names end up in the config dict with empty task lists while the task-assignment loop silently ignores them. The src/praisonai/praisonai/agents_generator.py — both Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[YAML config loaded] --> B{agents/roles list?}
B -- yes --> C[_list_to_dict\npreserve duplicates\nwith __dup_N suffix]
B -- no --> D[use existing dict]
C --> E{duplicate names?}
E -- STRICT=true --> F[raise ValueError]
E -- STRICT=false --> G[warn + suffix key\ne.g. researcher__dup_1]
G --> H[tasks list processing]
D --> H
H --> I{task.agent in bucket?}
I -- no --> J{STRICT=true?}
J -- yes --> K[raise ValueError]
J -- no --> L[warn + skip task]
I -- yes --> M[assign task to\nfirst matching agent\noriginal key only]
M --> N[__dup_N agents\norphaned — no tasks]
N --> O[_resolve_effective_tool_timeout]
O --> P{CLI tool_timeout?}
P -- yes --> Q[use CLI value]
P -- no --> R[min across all\nagent declarations]
R --> S[warn agents overridden\nby tighter budget]
Q --> T[build tools dict\nwith shared timeout]
S --> T
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[YAML config loaded] --> B{agents/roles list?}
B -- yes --> C[_list_to_dict\npreserve duplicates\nwith __dup_N suffix]
B -- no --> D[use existing dict]
C --> E{duplicate names?}
E -- STRICT=true --> F[raise ValueError]
E -- STRICT=false --> G[warn + suffix key\ne.g. researcher__dup_1]
G --> H[tasks list processing]
D --> H
H --> I{task.agent in bucket?}
I -- no --> J{STRICT=true?}
J -- yes --> K[raise ValueError]
J -- no --> L[warn + skip task]
I -- yes --> M[assign task to\nfirst matching agent\noriginal key only]
M --> N[__dup_N agents\norphaned — no tasks]
N --> O[_resolve_effective_tool_timeout]
O --> P{CLI tool_timeout?}
P -- yes --> Q[use CLI value]
P -- no --> R[min across all\nagent declarations]
R --> S[warn agents overridden\nby tighter budget]
Q --> T[build tools dict\nwith shared timeout]
S --> T
Reviews (2): Last reviewed commit: "fix: guard None roles/agents in timeout+..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tightest = float(min(timeouts)) | ||
| for name, entity in entities.items(): | ||
| v = _declared(entity) | ||
| if v is not None and float(v) != tightest: | ||
| self.logger.warning( | ||
| "Agent %r declared tool_timeout=%s but a shared tool dict " | ||
| "forces the tightest value %ss for the whole run.", | ||
| name, v, tightest, | ||
| ) | ||
| return tightest |
There was a problem hiding this comment.
min() timeout silently breaks slow-tool agents
An agent that explicitly declares tool_timeout: 60 to accommodate long-running operations (slow APIs, large file reads, compilation) will now be forced to timeout after 5 seconds if any peer agent in the same YAML declares tool_timeout: 5. Since the shared tool dict has a single effective timeout, min() guarantees the slow agent's tools will always time out prematurely rather than that the fast agent will ever wait slightly longer. The warning is emitted, but the run still proceeds — meaning the slow agent's tools silently fail on every invocation.
The original max() had the mirror problem (a 5 s agent inherits a 60 s budget and can get stuck), but that only wastes time; min() actively breaks correct configurations. Neither value is universally right for a shared dict, which is exactly why the per-agent restructure is needed. Until that is done, the safer fallback is max() (tools can always complete), with a visible warning that the effective timeout may be larger than declared.
| def _list_to_dict(entries: list, prefix: str, kind: str) -> dict: | ||
| """Convert a list of named entries to a dict, preserving duplicates. | ||
|
|
||
| Duplicate ``name`` keys would otherwise silently clobber each other. Instead | ||
| we raise under ``PRAISONAI_VALIDATE_STRICT`` or warn loudly and keep both by | ||
| suffixing the colliding key, so a multi-agent YAML never quietly shrinks. | ||
| """ | ||
| normalized: dict = {} | ||
| duplicates: list = [] | ||
| for i, entry in enumerate(entries): | ||
| if not isinstance(entry, dict): | ||
| continue | ||
| key = entry.get("name") or f"{prefix}_{i}" | ||
| if key in normalized: | ||
| duplicates.append(key) | ||
| key = f"{key}__dup_{i}" | ||
| normalized[key] = entry | ||
| if duplicates: | ||
| msg = f"Duplicate {kind} name(s) in YAML: {sorted(set(duplicates))}" | ||
| if _strict_validation_enabled(): | ||
| raise ValueError(msg) | ||
| logger.warning("%s — kept both by suffixing keys; rename to silence.", msg) | ||
| return normalized |
There was a problem hiding this comment.
__dup_i suffix but receives no task assignments
_list_to_dict stores duplicate agents under a suffixed key (e.g. researcher__dup_1), but the task-assignment loop at line 97 looks up agents by the value of task.get("agent"), which still holds the original name "researcher". The bucket lookup therefore always hits the first entry, and researcher__dup_1 ends up in the config dict with an empty tasks sub-dict. Downstream adapters that require at least one task per agent (or that iterate the role dict expecting all agents to be runnable) will encounter an orphaned, task-less agent. The original code silently collapsed to the last entry; the new code changes the failure mode rather than resolving it, and the warning that is emitted does not prevent the partial config from being passed to the framework layer.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3176 ✅Reviewed against AGENTS.md (lightweight-and-powerful, protocol-driven, backward-compatible) and all prior reviewer feedback. Fixes pushed to Verdict: APPROVE — scope-appropriate, backward-compatible, correctly routed (all changes in wrapper
|
| # | Source | Finding | Action |
|---|---|---|---|
| 1 | CodeRabbit | {**config.get("roles", {}), ...} → TypeError when YAML has roles: null |
Fixed — or {} fallback in _resolve_effective_tool_timeout (L813) and the same latent bug in _validate_cli_backend_compatibility (L859) |
| 2 | Greptile P2 | redundant inner import os in _strict_validation_enabled |
Fixed — os already imported at module top; removed |
Verified with a direct reproducer (roles: None → returns 7.0/None, no crash).
Reviewer points I evaluated and deliberately skipped (not bugs)
- Greptile P1 — revert
min()→max()timeout: Rejected. The issue author (repo owner, Wrapper gaps in src/praisonai/praisonai: silent YAML duplicate drops, global-max tool_timeout, and skill nested-path lockout #3175) explicitly endorsedmin()+warn as the correct minimal fix and calledmax()"the worst choice of the three" — a fast agent inheriting a 60s budget can hang, which violates multi-agent safe by default.min()bounds every tool and warns loudly on override. Keeping. The proper long-term answer (per-agent tool dict) is intentionally deferred — no live adapter consumer, so building it now would be scope creep. - Greptile P1 — orphaned
__dup_iagent gets no tasks: Rejected as "bug". This is the intended, endorsed behaviour — the issue asks to preserve both entries + warn loudly (raise underPRAISONAI_VALIDATE_STRICT) rather than silently collapse 3 agents → 1. A visible, task-less duplicate that emits a warning is the "loud, actionable" outcome the issue requires; matches the author's fix sketch exactly. - CodeRabbit inline (L814): Same finding as Github actions fix #1 above — resolved.
Testing
test_tool_timeout_enforcement.py— 10 passedtest_skills_cli.py+test_agents_yaml_validation.py+test_yaml_skills.py— 27 passed (37 total green)- Remaining collection errors in the broader suite are pre-existing env gaps (
pydantic/typer/tomlnot installed), unrelated to this PR.
Files modified
src/praisonai/praisonai/agents_generator.py(+3/−4)
No changes to praisonai-rust / praisonai-ts / praisonaiagents/ (out of scope).
Job run · branch claude/issue-3175-20260719-0817
…edundant os import
- _resolve_effective_tool_timeout and _validate_cli_backend_compatibility now
fall back to {} when roles/agents are explicit YAML nulls, preventing a
TypeError on dict unpacking (CodeRabbit).
- Remove redundant inner `import os` in _strict_validation_enabled; os is
already imported at module top (Greptile).
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3175
Summary
Wrapper-layer (
src/praisonai/praisonai/) fixes for three silent behavioural gaps, kept minimal and backward-compatible.1. Silent YAML duplicate collapse (
agents_generator.py)_normalize_yaml_configpreviously collapsed list-form agents/tasks with duplicatenamekeys silently — a 3-agent YAML could run as 1. Now duplicate agent/task names and unknown task→agent references raise underPRAISONAI_VALIDATE_STRICT=trueor warn loudly and preserve both entries by suffixing (name__dup_i). Correct YAML is unaffected.2. Per-agent
tool_timeoutcollapsed to globalmax()(agents_generator.py)_resolve_effective_tool_timeoutusedmax(), forcing a fast agent (5s) to inherit a slow agent's budget (60s). Switched to the tightest (min) value — the safe-by-default choice — and warns on every agent whose declared value is overridden. Avoided a per-agent tools-dict restructure (no live adapter consumer → scope creep).3. Nested skill paths blocked (
tools/skill_manage.py)_is_safe_pathdeny-listed/, blocking every canonical nested skill file (references/,scripts/,assets/). Now allows nested relative paths while still rejecting absolute paths,~, null bytes, and..segments. Real containment remains enforced by the callers'resolve().relative_to()check.Test plan
test_effective_timeout_uses_tightest_declared_role(max→min semantics)test_tool_timeout_enforcement.py— 10 passedGenerated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests