diff --git a/.beads/interactions.jsonl b/.beads/interactions.jsonl index aa59eb71..b3e43244 100644 --- a/.beads/interactions.jsonl +++ b/.beads/interactions.jsonl @@ -31,3 +31,13 @@ {"id":"int-47f29fcd4eb72c055882d94bfa857cc9","kind":"field_change","created_at":"2026-07-03T03:42:55.3794412Z","actor":"gerso","issue_id":"bd-6dn","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Implemented on claude/manager-mode-followups (a1d4fad, 6333777), merged into PR #111 (open, awaiting director merge); 702 tests green"}} {"id":"int-55233ecb5ca93873ebc945f863041274","kind":"field_change","created_at":"2026-07-03T03:42:56.5445665Z","actor":"gerso","issue_id":"bd-a63","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"Investigation complete: 4-conflict map + landing sequence in scratchpad hub-reconcile-report.md; execution requires hub session (31 unpushed commits + 1.9k uncommitted lines)"}} {"id":"int-b72aacf782dcdef7443894371b161eb6","kind":"field_change","created_at":"2026-07-06T15:51:44.4304982Z","actor":"gerso","issue_id":"bd-dm3","extra":{"field":"status","new_value":"closed","old_value":"in_progress","reason":"All 14 punch-list items resolved: L1 autouse _sandbox_home fixture, L2 quickstart project-root knowledge wiring, L3 --explain+--json JSON payload, L4 dotted-stem .md fallback, L5 expanduser on knowledge roots, L6 strict missing-root issue, L7 knowledge CLI docs section, L8 talent-manager alias claims deleted, L9 posix report_path, L10 dynamic warnings before static caveats, L11 reference count 20 synced, L12 mid-run backend-error semantics documented+tested, L13 golden-test env isolation, L14 writability caveat documented. Full-suite A/B confirmed zero regressions (175 pre-existing Windows-env failures identical at baseline)."}} +{"id":"int-06334511e4a7ee5bd6f72837d144a7cd","kind":"field_change","created_at":"2026-07-06T16:28:12.7195047Z","actor":"gerso","issue_id":"bd-6z9","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-a5dba7326af80aa10e526671c1ef9dbe","kind":"field_change","created_at":"2026-07-06T16:28:12.8863553Z","actor":"gerso","issue_id":"bd-5mq","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-33199b717ee0a725eb6966299717046e","kind":"field_change","created_at":"2026-07-06T16:28:13.0518024Z","actor":"gerso","issue_id":"bd-85c","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-1f2255c088aeaa55a294239ba5db4505","kind":"field_change","created_at":"2026-07-06T16:28:13.2106971Z","actor":"gerso","issue_id":"bd-6cr","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-6ce0f88e17f0bd8bac20d0636da07030","kind":"field_change","created_at":"2026-07-06T16:28:13.3781988Z","actor":"gerso","issue_id":"bd-j9f","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-c91c0187ad5d3b0cb3232ce7980984c6","kind":"field_change","created_at":"2026-07-06T16:28:13.5388979Z","actor":"gerso","issue_id":"bd-c40","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave (8a183a5..5dc5cc2); all 16 items verified FIXED by fable xhigh adversarial gate with pre-fix test-failure proof"}} +{"id":"int-4f5b85d4000aeccc178776da055232d0","kind":"field_change","created_at":"2026-07-06T21:40:04.2735651Z","actor":"gerso","issue_id":"bd-2hs","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed on claude/adversarial-fix-wave: conditional post-loop save in start() per fable consult ruling; OCC test green, team_readiness persistence proven covered"}} +{"id":"int-20cf10206ee551c564520b299b9cf8ea","kind":"field_change","created_at":"2026-07-06T21:45:23.9779044Z","actor":"gerso","issue_id":"bd-ftd","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Docs synced on claude/adversarial-fix-wave commit for bd-ftd; agent also closed two pre-existing doc holes (forge 422s undocumented, baton doctor absent from cli-reference)"}} +{"id":"int-a18184d525369f5ee8a65d63deef9954","kind":"field_change","created_at":"2026-07-06T23:17:53.1621743Z","actor":"gerso","issue_id":"bd-rbt","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Tests made hermetic via KeywordClassifier injection per established pattern; production code correct. 3 tests green, planning suites 209 passed."}} +{"id":"int-934a4d09e996ae26efdcc92d16e1b860","kind":"field_change","created_at":"2026-07-06T23:47:30.1835886Z","actor":"gerso","issue_id":"bd-pz4","extra":{"field":"status","new_value":"closed","old_value":"open","reason":"Fixed in two parts: RiskStage presence-based Review/Audit slot guarantee (2720191) + decomposition override-path roster/phase coupling per fable consult adjudication. e2e 18/18, planning 211 green, fan-out guard contract preserved."}} diff --git a/agent_baton/api/routes/pmo.py b/agent_baton/api/routes/pmo.py index e66608e5..257527b8 100644 --- a/agent_baton/api/routes/pmo.py +++ b/agent_baton/api/routes/pmo.py @@ -2314,6 +2314,11 @@ async def forge_signal( try: plan = forge.signal_to_plan(signal_id=signal_id, project_id=req.project_id) + except PlanQualityError as exc: + raise HTTPException( + status_code=422, + detail=plan_quality_error_detail(exc), + ) from exc except Exception as exc: raise HTTPException( status_code=500, diff --git a/agent_baton/cli/commands/diagnostics_cmd.py b/agent_baton/cli/commands/diagnostics_cmd.py index ed86ee39..16bab597 100644 --- a/agent_baton/cli/commands/diagnostics_cmd.py +++ b/agent_baton/cli/commands/diagnostics_cmd.py @@ -790,12 +790,30 @@ def _check_terminology() -> DoctorCheck: ) +def _with_fallback_caveat(message: str, details: dict[str, Any]) -> str: + """Append a caveat when the plan came from an unguided fallback guess. + + Only applies once a plan was actually found (``plan_path`` set) — a + ``fallback-first-found`` selection with no plan at all is just "no plan", + not a guess worth flagging. + """ + if details.get("plan_selection") == "fallback-first-found" and details.get( + "plan_path" + ): + return ( + f"{message} (caveat: no active task resolved; validating the " + "first saved plan found, not necessarily the current one)" + ) + return message + + def _check_planner_validation(project_root: Path) -> DoctorCheck: plan_candidates = _saved_plan_candidates(project_root) plan_path, active_task_state = _select_saved_plan_for_validation(project_root) details: dict[str, Any] = { "active_task_id": active_task_state["active_task_id"], "active_task_source": active_task_state["active_task_source"], + "plan_selection": active_task_state["plan_selection"], "plan_candidates": [str(path) for path in plan_candidates], "plan_path": None, "machine_plan_importable": False, @@ -865,7 +883,9 @@ def _check_planner_validation(project_root: Path) -> DoctorCheck: id="planner_validation", label="Planner validation", status="error", - message=f"Saved plan could not be parsed: {exc}", + message=_with_fallback_caveat( + f"Saved plan could not be parsed: {exc}", details + ), details=details, ) @@ -878,7 +898,9 @@ def _check_planner_validation(project_root: Path) -> DoctorCheck: id="planner_validation", label="Planner validation", status="error", - message="Saved plan JSON has invalid top-level shape", + message=_with_fallback_caveat( + "Saved plan JSON has invalid top-level shape", details + ), details=details, ) @@ -891,7 +913,9 @@ def _check_planner_validation(project_root: Path) -> DoctorCheck: id="planner_validation", label="Planner validation", status="error", - message=f"Saved plan validation could not run: {exc}", + message=_with_fallback_caveat( + f"Saved plan validation could not run: {exc}", details + ), details=details, ) @@ -912,9 +936,10 @@ def _check_planner_validation(project_root: Path) -> DoctorCheck: id="planner_validation", label="Planner validation", status="error", - message=( + message=_with_fallback_caveat( f"Saved plan validation found {error_count} errors and " - f"{warning_count} warnings" + f"{warning_count} warnings", + details, ), details=details, ) @@ -923,14 +948,18 @@ def _check_planner_validation(project_root: Path) -> DoctorCheck: id="planner_validation", label="Planner validation", status="warning", - message=f"Saved plan validation found {warning_count} warnings", + message=_with_fallback_caveat( + f"Saved plan validation found {warning_count} warnings", details + ), details=details, ) return DoctorCheck( id="planner_validation", label="Planner validation", status="ok", - message="Saved plan validation passed with no findings", + message=_with_fallback_caveat( + "Saved plan validation passed with no findings", details + ), details=details, ) @@ -1062,6 +1091,7 @@ def _select_saved_plan_for_validation( "active_task_id": active_task_id, "active_task_source": active_task_source, "active_plan_missing": False, + "plan_selection": "active-task" if active_task_id else "fallback-first-found", **active_task_details, } if active_task_id: @@ -1097,7 +1127,13 @@ def _resolve_active_task_for_validation( def _read_active_task_id_from_sqlite( context_root: Path, ) -> tuple[str | None, dict[str, Any]]: - db_path = context_root / "baton.db" + # Honour BATON_DB_PATH (mirrors bead_cmd._resolve_db_path's override + # precedence) so doctor probes the same DB the rest of the CLI uses. + override = os.environ.get("BATON_DB_PATH", "").strip() + if override: + db_path = Path(override).expanduser().resolve() + else: + db_path = context_root / "baton.db" from agent_baton.core.storage.active_task import ( read_active_task_id_from_db_copy, ) diff --git a/agent_baton/cli/commands/execution/execute.py b/agent_baton/cli/commands/execution/execute.py index 04894fe7..e9e61756 100644 --- a/agent_baton/cli/commands/execution/execute.py +++ b/agent_baton/cli/commands/execution/execute.py @@ -30,6 +30,7 @@ from agent_baton.cli.errors import user_error, validation_error from agent_baton.core.engine.errors import ExecutionVetoed from agent_baton.core.engine.executor import ExecutionEngine +from agent_baton.core.engine.team_backends import UnknownTeamBackendError from agent_baton.core.engine.persistence import StatePersistence from agent_baton.core.events.bus import EventBus from agent_baton.core.storage import get_project_storage @@ -804,6 +805,18 @@ def _print_action(action: dict, *, terse: bool = False) -> None: def handler(args: argparse.Namespace) -> None: + # UnknownTeamBackendError can surface from any engine call that walks a + # team step (start / next / resume / run) when BATON_TEAMS_BACKEND is + # unknown under BATON_TEAMS_BACKEND_STRICT=1. Catch it at the command + # entry so the CLI prints a clean message (matching the API's str(exc) + # mapping) and exits non-zero instead of surfacing a traceback. + try: + _dispatch(args) + except UnknownTeamBackendError as exc: + user_error(str(exc)) + + +def _dispatch(args: argparse.Namespace) -> None: if args.subcommand is None: # bd-8944: consolidated single validation_error with the full list of # registered subcommands (removed stale duplicate that was unreachable). diff --git a/agent_baton/cli/commands/knowledge/doctor_cmd.py b/agent_baton/cli/commands/knowledge/doctor_cmd.py index b107d14c..11001e7e 100644 --- a/agent_baton/cli/commands/knowledge/doctor_cmd.py +++ b/agent_baton/cli/commands/knowledge/doctor_cmd.py @@ -262,8 +262,15 @@ def validate_knowledge_roots( } if require_roots: + seen_roots: set[Path] = set() for root in roots: resolved = root.expanduser() + # De-dup consistent with _unique_existing_roots so a repeated + # --knowledge-root argument doesn't inflate the warning count. + dedup_key = resolved.resolve() + if dedup_key in seen_roots: + continue + seen_roots.add(dedup_key) if not resolved.is_dir(): issues.append(_issue( code="missing-root", @@ -278,95 +285,126 @@ def validate_knowledge_roots( for root in _unique_existing_roots(roots): summary["roots"] += 1 - for pack_dir in sorted(p for p in root.iterdir() if p.is_dir()): - summary["packs"] += 1 - manifest, manifest_ok = _read_manifest(pack_dir, issues) - default_delivery = str( - manifest.get("default_delivery") or "reference" - ).strip().lower() - - declared_paths = _declared_doc_paths(manifest) - for rel_path in declared_paths: - doc_path = pack_dir / rel_path - if not _declared_doc_exists(pack_dir, rel_path): - issues.append(_issue( - code="missing-declared-file", - path=doc_path, - pack=pack_dir.name, - message=( - f"Pack '{pack_dir.name}' declares missing document " - f"'{rel_path}'. Edit {pack_dir / 'knowledge.yaml'} " - "or create that file." - ), - )) - - if manifest_ok and not str(manifest.get("description") or "").strip(): - issues.append(_issue( - code="empty-pack-description", - path=pack_dir / "knowledge.yaml", - pack=pack_dir.name, - message=( - f"Pack '{pack_dir.name}' has an empty description. " - f"Edit {pack_dir / 'knowledge.yaml'} and add a " - "description for search and resolver matching." - ), - )) + try: + pack_candidates = sorted(root.iterdir()) + except OSError as exc: + issues.append(_issue( + code="unreadable-pack", + path=root, + pack="", + message=( + f"Knowledge root '{root}' could not be listed: {exc}. " + "Check directory permissions and re-run doctor." + ), + )) + continue - names: dict[str, list[Path]] = {} - for doc_path in sorted(pack_dir.glob("*.md")): - summary["documents"] += 1 - doc_metadata = _read_doc_metadata( - doc_path, pack_dir.name, issues - ) - if doc_metadata is None: + for pack_dir in pack_candidates: + # Isolate per-pack filesystem failures (Windows ACLs, cloud-sync + # placeholders) so one unreadable pack doesn't abort the whole + # doctor run — mirror KnowledgeRegistry.load_directory's guard. + try: + if not pack_dir.is_dir(): continue - doc_name, metadata, raw = doc_metadata - names.setdefault(doc_name, []).append(doc_path) - - description = str(metadata.get("description") or "").strip() - if not description: + summary["packs"] += 1 + manifest, manifest_ok = _read_manifest(pack_dir, issues) + default_delivery = str( + manifest.get("default_delivery") or "reference" + ).strip().lower() + + declared_paths = _declared_doc_paths(manifest) + for rel_path in declared_paths: + doc_path = pack_dir / rel_path + if not _declared_doc_exists(pack_dir, rel_path): + issues.append(_issue( + code="missing-declared-file", + path=doc_path, + pack=pack_dir.name, + message=( + f"Pack '{pack_dir.name}' declares missing document " + f"'{rel_path}'. Edit {pack_dir / 'knowledge.yaml'} " + "or create that file." + ), + )) + + if manifest_ok and not str(manifest.get("description") or "").strip(): issues.append(_issue( - code="empty-doc-description", - path=doc_path, + code="empty-pack-description", + path=pack_dir / "knowledge.yaml", pack=pack_dir.name, - doc=doc_name, message=( - f"Document '{doc_name}' has an empty description. " - f"Edit {doc_path} frontmatter and add description." + f"Pack '{pack_dir.name}' has an empty description. " + f"Edit {pack_dir / 'knowledge.yaml'} and add a " + "description for search and resolver matching." ), )) - if _is_large_inline_candidate( - doc_path, raw, default_delivery=default_delivery - ): + names: dict[str, list[Path]] = {} + for doc_path in sorted(pack_dir.glob("*.md")): + summary["documents"] += 1 + doc_metadata = _read_doc_metadata( + doc_path, pack_dir.name, issues + ) + if doc_metadata is None: + continue + doc_name, metadata, raw = doc_metadata + names.setdefault(doc_name, []).append(doc_path) + + description = str(metadata.get("description") or "").strip() + if not description: + issues.append(_issue( + code="empty-doc-description", + path=doc_path, + pack=pack_dir.name, + doc=doc_name, + message=( + f"Document '{doc_name}' has an empty description. " + f"Edit {doc_path} frontmatter and add description." + ), + )) + + if _is_large_inline_candidate( + doc_path, raw, default_delivery=default_delivery + ): + issues.append(_issue( + code="large-inline-candidate", + path=doc_path, + pack=pack_dir.name, + doc=doc_name, + message=( + f"Document '{doc_name}' is too large for likely " + f"inline delivery. Edit {doc_path} to shorten it " + f"or edit {pack_dir / 'knowledge.yaml'} and set " + "default_delivery: reference." + ), + )) + + for doc_name, paths in sorted(names.items()): + if len(paths) <= 1: + continue + joined = ", ".join(str(p) for p in paths) issues.append(_issue( - code="large-inline-candidate", - path=doc_path, + code="duplicate-doc-name", + path=paths[0], pack=pack_dir.name, doc=doc_name, message=( - f"Document '{doc_name}' is too large for likely " - f"inline delivery. Edit {doc_path} to shorten it " - f"or edit {pack_dir / 'knowledge.yaml'} and set " - "default_delivery: reference." + f"Document name '{doc_name}' is duplicated in " + f"{joined}. Edit one frontmatter name field so each " + "document name is unique within the pack." ), )) - - for doc_name, paths in sorted(names.items()): - if len(paths) <= 1: - continue - joined = ", ".join(str(p) for p in paths) + except OSError as exc: issues.append(_issue( - code="duplicate-doc-name", - path=paths[0], + code="unreadable-pack", + path=pack_dir, pack=pack_dir.name, - doc=doc_name, message=( - f"Document name '{doc_name}' is duplicated in " - f"{joined}. Edit one frontmatter name field so each " - "document name is unique within the pack." + f"Pack '{pack_dir.name}' could not be validated: {exc}. " + "Check directory permissions and re-run doctor." ), )) + continue summary["warnings"] = len(issues) return issues, summary @@ -472,13 +510,21 @@ def _declared_doc_paths(manifest: dict[str, Any]) -> list[str]: return paths +# Suffixes that are plausible standalone file types in their own right. +# A declaration ending in one of these (e.g. "config.json") should not +# be satisfied by a shadow "config.json.md" — only genuinely extensionless +# stems (e.g. "notes.v2") get the +.md fallback. +_KNOWN_NON_MD_EXTENSIONS = { + ".json", ".yaml", ".yml", ".txt", ".py", ".toml", ".csv", +} + + def _declared_doc_exists(pack_dir: Path, rel_path: str) -> bool: doc_path = pack_dir / rel_path if doc_path.is_file(): return True - # Only treat the declaration as extensionless when it does not already - # end in .md — Path.suffix would misread dotted stems like "notes.v2". - if rel_path.lower().endswith(".md"): + suffix = doc_path.suffix.lower() + if suffix == ".md" or suffix in _KNOWN_NON_MD_EXTENSIONS: return False return doc_path.with_name(doc_path.name + ".md").is_file() diff --git a/agent_baton/core/engine/executor.py b/agent_baton/core/engine/executor.py index 5d320a9e..c8ce88fa 100644 --- a/agent_baton/core/engine/executor.py +++ b/agent_baton/core/engine/executor.py @@ -2035,8 +2035,23 @@ def start(self, plan: MachinePlan) -> ExecutionAction: state.task_id, exc_info=True, ) + team_readiness_before = dict( + state.plan.plan_diagnostics.get("team_readiness", {}) + ) action = self._drive_resolver_loop(state) - self._save_execution(state) + # Conditional save (mirrors next_actions() at ~2034-2038): the + # pre-loop save above already persisted the run-start CAS bump, and + # _dispatch_action self-persists its own status mutation inline. A + # non-team plan therefore has nothing new to save post-loop. Team + # plans mutate plan_diagnostics["team_readiness"] inside + # _team_dispatch_action; only in that case do we need a second save + # so team_readiness lands on disk. An unconditional save here would + # double-bump the OCC version (bd-2hs). + if ( + state.plan.plan_diagnostics.get("team_readiness", {}) + != team_readiness_before + ): + self._save_execution(state) return action def next_action(self) -> ExecutionAction: @@ -4816,7 +4831,9 @@ def resume(self) -> ExecutionAction: except Exception as _be_resume_exc: # pragma: no cover _log.debug("BudgetEnforcer resume restore skipped (non-fatal): %s", _be_resume_exc) - return self._drive_resolver_loop(state) + action = self._drive_resolver_loop(state) + self._save_execution(state) + return action def recover_dispatched_steps(self) -> int: """Clear stale dispatched-step markers for crash recovery. diff --git a/agent_baton/core/engine/planning/stages/decomposition.py b/agent_baton/core/engine/planning/stages/decomposition.py index de73fdb3..a91da5a3 100644 --- a/agent_baton/core/engine/planning/stages/decomposition.py +++ b/agent_baton/core/engine/planning/stages/decomposition.py @@ -20,6 +20,7 @@ from agent_baton.core.engine.planning.rules.phase_templates import PHASE_NAMES as _PHASE_NAMES from agent_baton.core.engine.planning.services import PlannerServices from agent_baton.core.engine.planning.utils.phase_builder import ( + _normalize_phase_name, apply_pattern, assign_agents_to_phases, build_compound_phases, @@ -28,6 +29,7 @@ enrich_phases, phases_from_dicts, ) +from agent_baton.core.orchestration.router import REVIEWER_AGENTS if TYPE_CHECKING: from agent_baton.models.execution import PlanPhase @@ -40,6 +42,13 @@ class DecompositionStage: name = "decomposition" + # Same derivation ValidationStage._REVIEWER_BASES / RiskStage._REVIEWER_BASES + # use (validation.py:143, risk.py:48) — keeps "is a reviewer-class agent" + # agreement across the roster-filtering, safety-injection, and gate stages. + # ``auditor`` is excluded: it is governed by the separate Audit + # phase/gate, not the Review phase this stage reasons about. + _REVIEWER_BASES = REVIEWER_AGENTS - {"auditor"} + def run(self, draft: PlanDraft, services: PlannerServices) -> PlanDraft: # Step 9+9b — build phase list. draft.plan_phases = self._build_phases( @@ -143,6 +152,32 @@ def _build_phases( # Explicit complexity override — scale phases to match. from agent_baton.core.engine.classifier import KeywordClassifier as _KC complexity_phases = _KC()._select_phases(inferred_type, inferred_complexity, _PHASE_NAMES) + + # The complexity-driven phase count and the roster are computed + # independently on this path (phases via KeywordClassifier, + # roster via rules/default_agents.py's static DEFAULT_AGENTS). + # KeywordClassifier keeps its own roster/phase selection paired + # (_select_agents drops reviewer-class agents at the same + # complexity tiers _select_phases drops "Review"), but this + # override path does not inherit that pairing. If the resulting + # phase list has no Review phase, drop reviewer-class agents + # (except auditor, which is governed by the separate Audit + # phase/gate) from the roster before phases are built, so a + # rostered reviewer never ends up stranded inside an Implement + # phase where ValidationStage's review_missing gate would + # reject the plan. + has_review_phase = any( + _normalize_phase_name(name) == "review" for name in complexity_phases + ) + if not has_review_phase: + filtered_agents = [ + a for a in resolved_agents + if a.split("--")[0] not in self._REVIEWER_BASES + ] + if filtered_agents != resolved_agents: + resolved_agents = filtered_agents + draft.resolved_agents = filtered_agents + plan_phases = build_phases_for_names(complexity_phases, resolved_agents, task_summary, registry) else: plan_phases = default_phases(inferred_type, resolved_agents, task_summary, registry) diff --git a/agent_baton/core/engine/planning/stages/risk.py b/agent_baton/core/engine/planning/stages/risk.py index 6da4f025..f75a287e 100644 --- a/agent_baton/core/engine/planning/stages/risk.py +++ b/agent_baton/core/engine/planning/stages/risk.py @@ -28,6 +28,7 @@ requires_audit_coverage, select_git_strategy, ) +from agent_baton.core.orchestration.router import REVIEWER_AGENTS from agent_baton.models.enums import RiskLevel if TYPE_CHECKING: @@ -41,6 +42,11 @@ class RiskStage: name = "risk" + # Same derivation ValidationStage._REVIEWER_BASES uses (validation.py:143) + # so "is a reviewer-class agent already on the roster" agrees between the + # gate that blocks the plan and the stage that guarantees a phase slot. + _REVIEWER_BASES = REVIEWER_AGENTS - {"auditor"} + # ------------------------------------------------------------------ # Public entry point # ------------------------------------------------------------------ @@ -248,14 +254,29 @@ def _base(name: str) -> str: ) injected_auditor = True - injected_any = injected_reviewer or injected_auditor + # Presence flags computed from the PRE-INJECTION roster (current_bases, + # captured above before either append happens) OR'd with this call's + # own injection — i.e. true whenever a reviewer/auditor is (or is about + # to be) on the roster, regardless of whether THIS call is what put it + # there. This must stay in lockstep with ValidationStage._REVIEWER_BASES + # (validation.py:143) / _review_required (validation.py:485-489), which + # gate on roster membership alone — not on "did RiskStage just inject + # it". Without this, a code-reviewer that was already on the roster at + # medium/low risk (no injection needed) leaves classified_phases without + # a Review slot, so the phase builder force-lands the reviewer in the + # Implement team-step and consolidate_team_step() (phase_builder.py) + # filters it back out — zero review coverage, hard-blocked plan. + reviewer_present = injected_reviewer or bool( + current_bases & self._REVIEWER_BASES + ) + auditor_present = injected_auditor or ("auditor" in current_bases) - # When agents were injected, guarantee that review-type phases exist in - # classified_phases so each injected safety agent has a home. + # When a reviewer/auditor is present, guarantee that review-type + # phases exist in classified_phases so it has a home. # # Background: ClassificationStage may produce a medium-complexity phase # list (e.g. ["Design", "Implement", "Test"]) that strips Review before - # risk is known. Without this correction the injected agents are + # risk is known. Without this correction the safety agent is # force-assigned to the Implement team step and then silently filtered # by the phase-builder's reviewer-exclusion guard. # @@ -263,22 +284,19 @@ def _base(name: str) -> str: # most ONE primary agent per phase in Pass 1. If both code-reviewer and # auditor are on the roster they need SEPARATE review-type phase slots — # one takes "Review" and the other takes "Audit". - if injected_any and draft.classified_phases is not None: + if (reviewer_present or auditor_present) and draft.classified_phases is not None: new_phases = list(draft.classified_phases) added: list[str] = [] - if injected_reviewer and "Review" not in new_phases: + if reviewer_present and "Review" not in new_phases: new_phases.append("Review") added.append("Review") - if injected_auditor: + if auditor_present: # auditor needs its own phase slot distinct from "Review" # (which code-reviewer claims). Use "Audit" — it maps to the # review ideal-roles table and is not blocked anywhere. - reviewer_in_roster = ( - "code-reviewer" in current_bases or injected_reviewer - ) - if reviewer_in_roster and "Audit" not in new_phases: + if reviewer_present and "Audit" not in new_phases: new_phases.append("Audit") added.append("Audit") elif "Review" not in new_phases: diff --git a/agent_baton/core/engine/planning/stages/validation.py b/agent_baton/core/engine/planning/stages/validation.py index 78c60245..3d14edb1 100644 --- a/agent_baton/core/engine/planning/stages/validation.py +++ b/agent_baton/core/engine/planning/stages/validation.py @@ -44,7 +44,10 @@ from typing import TYPE_CHECKING from agent_baton.core.engine.planning.draft import PlanDraft -from agent_baton.core.engine.planning.rules.phase_roles import PHASE_BLOCKED_ROLES +from agent_baton.core.engine.planning.rules.phase_roles import ( + IMPLEMENT_PHASE_NAMES, + PHASE_BLOCKED_ROLES, +) from agent_baton.core.engine.planning.services import PlannerServices from agent_baton.core.engine.planning.utils.phase_builder import ( consolidate_team_step, @@ -70,6 +73,26 @@ logger = logging.getLogger(__name__) +def _collect_member_agent_names(member: object) -> list[str]: + """Collect ``agent_name`` from a TeamMember and its full ``sub_team`` tree. + + ``TeamMember.sub_team`` (models/execution.py) is self-referential and + unbounded, so this recurses to arbitrary depth. Used by both + ``ValidationStage._step_agent_bases`` (coverage/mismatch checks) and + ``_resolved_agents_from_plan`` so the two never disagree about which + agents a team step contains — a depth-1-only walk let depth-2+ auditors + trigger false ``audit_missing`` blocks and depth-2+ reviewers evade the + reviewer-in-implement check. + """ + names: list[str] = [] + name = str(getattr(member, "agent_name", "") or "") + if name: + names.append(name) + for nested in getattr(member, "sub_team", []) or []: + names.extend(_collect_member_agent_names(nested)) + return names + + class PlanQualityError(RuntimeError): """Raised by ValidationStage when the effective quality gate blocks.""" @@ -111,14 +134,12 @@ class ValidationStage: _DEV_MODE_ENV = "BATON_DEV_MODE" _WARN_ONLY_ENV = "BATON_PLANNER_WARN_ONLY" _TRUTHY = frozenset({"1", "true", "yes", "on"}) - _IMPLEMENT_PHASE_KEYS = frozenset({ - "implement", - "implementation", - "fix", - "build", - "develop", - "development", - }) + # Derived from the canonical implement-phase table so the reviewer-in- + # implement check stays in lockstep with routing (which folds + # implementation/development variants via ``_phase_key``). Deriving here + # keeps the documentation archetype's ``Draft`` phase in scope instead of + # silently dropping it (phase_roles.IMPLEMENT_PHASE_NAMES includes "draft"). + _IMPLEMENT_PHASE_KEYS = IMPLEMENT_PHASE_NAMES _REVIEWER_BASES = REVIEWER_AGENTS - {"auditor"} def run(self, draft: PlanDraft, services: PlannerServices) -> PlanDraft: # Step 10+11+11b — score check, budget tier, policy validation. @@ -211,8 +232,19 @@ def _check_scores( draft.policy_violations = validate_agents_against_policy( resolved_agents, policy_set, plan_phases, services.policy_engine ) - except Exception: - pass + except Exception as exc: + # Non-fatal: a policy-engine failure must not abort planning, + # but swallowing it silently voids the policy-driven + # audit-requirement branch (risk_and_policy.py:139-149). Make + # it visible so the missing audit gate is traceable. + logger.warning( + "planner.validation: policy validation failed for task %s " + "— policy-driven audit requirement not evaluated: %s", + draft.task_id, exc, exc_info=True, + ) + draft.score_warnings.append( + f"policy_validation_failed: {exc}" + ) return budget_tier @@ -383,7 +415,12 @@ def _detect_defects(self, draft: PlanDraft) -> list[PlanDefect]: f"task_id={draft.task_id} agents={sorted(agent_bases)} " f"{audit_requirement}. " "Compliance or auditor-routed plans require Audit coverage. " - "Remediation: add a terminal Audit phase with an auditor step." + "Remediation: add the auditor agent to the roster and a " + "terminal Audit phase with an auditor step. Headless/forge " + "plans (validate_assembled_plan) are NOT auto-remediated the " + "way the interactive RiskStage safety-roster path is — " + "regenerate the plan with the auditor included so the " + "auditor is guaranteed on both paths (parity of outcome)." ), )) @@ -516,13 +553,8 @@ def _step_agent_bases(self, step: object) -> set[str]: if agent_name: bases.add(agent_name.split("--")[0]) for member in getattr(step, "team", []) or []: - member_name = getattr(member, "agent_name", "") - if member_name: - bases.add(member_name.split("--")[0]) - for nested in getattr(member, "sub_team", []) or []: - nested_name = getattr(nested, "agent_name", "") - if nested_name: - bases.add(nested_name.split("--")[0]) + for name in _collect_member_agent_names(member): + bases.add(name.split("--")[0]) return bases @@ -533,15 +565,11 @@ def _append(agent_name: str) -> None: if agent_name and agent_name != "team": agents.append(agent_name) - def _collect_member_agents(member: object) -> None: - _append(str(getattr(member, "agent_name", "") or "")) - for nested in getattr(member, "sub_team", []) or []: - _collect_member_agents(nested) - for step in plan.all_steps: _append(step.agent_name) for member in getattr(step, "team", []) or []: - _collect_member_agents(member) + for name in _collect_member_agent_names(member): + _append(name) return list(dict.fromkeys(agents)) diff --git a/agent_baton/core/engine/planning/utils/risk_and_policy.py b/agent_baton/core/engine/planning/utils/risk_and_policy.py index c2cf4350..8f8a7b36 100644 --- a/agent_baton/core/engine/planning/utils/risk_and_policy.py +++ b/agent_baton/core/engine/planning/utils/risk_and_policy.py @@ -136,6 +136,19 @@ def audit_coverage_requirement( if preset.lower() == "regulated data": return f"guardrail_preset={preset}" + # Pack-classified regulated tasks carry a ``pack:`` preset rather + # than the ``Regulated Data`` literal (classifier.py:317-328, packs.py:120), + # so the exact-match branch above misses them. Risk-gate to HIGH/CRITICAL + # so only genuinely sensitive pack tasks pull in an auditor. ``risk_level`` + # is a RiskLevel enum on a live ClassificationResult but a string when the + # classification is rebuilt from a persisted plan (_classification_from_plan); + # coerce via ``.value`` before comparing. No registry lookups here. + if preset.startswith("pack:"): + risk = getattr(classification, "risk_level", None) + risk_name = str(getattr(risk, "value", risk) or "").upper() + if risk_name in ("HIGH", "CRITICAL"): + return f"guardrail_preset={preset}" + for violation in policy_violations or []: rule = getattr(violation, "rule", None) if ( diff --git a/agent_baton/core/engine/team_backends.py b/agent_baton/core/engine/team_backends.py index 8b8582fb..9e05df57 100644 --- a/agent_baton/core/engine/team_backends.py +++ b/agent_baton/core/engine/team_backends.py @@ -318,13 +318,17 @@ def _audit_step_agents( team_context_root.parent.parent / "agents", # repo agents/ ] for agents_dir in candidate_dirs: + # Honor the FIRST EXISTING directory authoritatively: a present but + # clean .claude/agents must not be overridden by a stale divergent + # repo-level agents/ copy (and vice versa). Only fall through when + # the higher-priority directory is absent. + if not agents_dir.exists(): + continue try: flagged = audit_agents_for_teammate_safety(agents_dir) except Exception: # noqa: BLE001 — degrade loudly, never throw continue - scoped = {a: f for a, f in flagged.items() if a in team_agents} - if scoped: - return scoped + return {a: f for a, f in flagged.items() if a in team_agents} return {} def hook_record_command( diff --git a/agent_baton/core/storage/migrate.py b/agent_baton/core/storage/migrate.py index 0bfc908b..82831dc6 100644 --- a/agent_baton/core/storage/migrate.py +++ b/agent_baton/core/storage/migrate.py @@ -712,14 +712,20 @@ def _insert_execution( ) new_execution = cur.rowcount > 0 - # plans row + # plans row. Column list mirrors sqlite_backend._upsert_plan + # (minus release_id, which legacy JSON state never carried and + # which is owned exclusively by release_store.tag_plan/untag_plan). conn.execute( """ INSERT OR IGNORE INTO plans (task_id, task_summary, risk_level, budget_tier, execution_mode, git_strategy, shared_context, - pattern_source, plan_markdown, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + pattern_source, plan_markdown, created_at, + explicit_knowledge_packs, explicit_knowledge_docs, + intervention_level, task_type, + classification_signals, classification_confidence, + manager_mode, plan_diagnostics) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """, ( plan.task_id, @@ -732,6 +738,17 @@ def _insert_execution( plan.pattern_source, plan.to_markdown(), plan.created_at, + json.dumps(plan.explicit_knowledge_packs), + json.dumps(plan.explicit_knowledge_docs), + plan.intervention_level, + plan.task_type, + plan.classification_signals, + plan.classification_confidence, + # v40-ish (manager-mode PMO layer): legacy JSON state predates + # this field on older MachinePlan objects, so fall back to + # the model default (False) via getattr for tolerance. + int(getattr(plan, "manager_mode", False)), + json.dumps(plan.plan_diagnostics), ), ) diff --git a/agent_baton/core/storage/sqlite_backend.py b/agent_baton/core/storage/sqlite_backend.py index 4e20e0d5..0d48f825 100644 --- a/agent_baton/core/storage/sqlite_backend.py +++ b/agent_baton/core/storage/sqlite_backend.py @@ -2209,9 +2209,14 @@ def _upsert_plan(conn: sqlite3.Connection, plan: "MachinePlan") -> None: # noqa ``with conn:`` transaction block managed by the caller. plan: The plan to persist. """ + # INSERT ... ON CONFLICT DO UPDATE (not INSERT OR REPLACE) so that an + # existing row's ``release_id`` survives a re-save. INSERT OR REPLACE + # is DELETE+INSERT under the hood (task_id is PRIMARY KEY), which wipes + # any column not in this statement's list -- including release_id, + # which is owned exclusively by release_store.tag_plan/untag_plan. conn.execute( """ - INSERT OR REPLACE INTO plans + INSERT INTO plans (task_id, task_summary, risk_level, budget_tier, execution_mode, git_strategy, shared_context, pattern_source, plan_markdown, created_at, @@ -2220,6 +2225,24 @@ def _upsert_plan(conn: sqlite3.Connection, plan: "MachinePlan") -> None: # noqa classification_signals, classification_confidence, manager_mode, plan_diagnostics) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(task_id) DO UPDATE SET + task_summary = excluded.task_summary, + risk_level = excluded.risk_level, + budget_tier = excluded.budget_tier, + execution_mode = excluded.execution_mode, + git_strategy = excluded.git_strategy, + shared_context = excluded.shared_context, + pattern_source = excluded.pattern_source, + plan_markdown = excluded.plan_markdown, + created_at = excluded.created_at, + explicit_knowledge_packs = excluded.explicit_knowledge_packs, + explicit_knowledge_docs = excluded.explicit_knowledge_docs, + intervention_level = excluded.intervention_level, + task_type = excluded.task_type, + classification_signals = excluded.classification_signals, + classification_confidence = excluded.classification_confidence, + manager_mode = excluded.manager_mode, + plan_diagnostics = excluded.plan_diagnostics """, ( plan.task_id, diff --git a/docs/api-reference.md b/docs/api-reference.md index cab38768..6a569123 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1588,6 +1588,7 @@ returned for review but NOT saved to disk. |---|---| | `400` | Invalid request | | `404` | Project not found | +| `422` | Generated plan failed the quality gate (structured `plan_quality_error` detail — see [§8 Plan Quality Errors](#plan-quality-errors-422)) | | `500` | Plan creation failed | **Example:** @@ -1699,6 +1700,7 @@ Each `InterviewAnswerPayload`: | Status | Condition | |---|---| | `404` | Project not found | +| `422` | Regenerated plan failed the quality gate (structured `plan_quality_error` detail — see [§8 Plan Quality Errors](#plan-quality-errors-422)) | | `500` | Regeneration failed | --- @@ -1853,6 +1855,7 @@ plan to the project's team-context, and updates the signal status to `triaged`. | Status | Condition | |---|---| | `404` | Project or signal not found | +| `422` | Generated plan failed the quality gate (structured `plan_quality_error` detail — see [§8 Plan Quality Errors](#plan-quality-errors-422)) | | `500` | Forge triaging failed | --- @@ -2711,6 +2714,42 @@ For validation errors (422), the response includes field-level details: } ``` +### Plan Quality Errors (422) + +`POST /pmo/forge/plan`, `POST /pmo/forge/regenerate`, and +`POST /pmo/signals/{signal_id}/forge` run the generated plan through the +planner's quality gate (`ValidationStage`) before returning it. If the +gate rejects the plan, these three routes return `422` with a structured +`detail` body (built by `plan_quality_error_detail()`) instead of an +opaque `500`: + +```json +{ + "detail": { + "error": "plan_quality_error", + "message": "Plan task-abc123 blocked by ValidationStage: [critical] review_missing: task_id=task-abc123 risk=high agents=['backend-engineer--python']. High-risk or reviewer-routed plans require Review coverage. Remediation: add a terminal Review phase with code-reviewer or security-reviewer steps.", + "defects": [ + { + "code": "review_missing", + "severity": "critical", + "message": "task_id=task-abc123 risk=high agents=['backend-engineer--python']. High-risk or reviewer-routed plans require Review coverage.", + "remediation": "add a terminal Review phase with code-reviewer or security-reviewer steps." + } + ] + } +} +``` + +| Field | Type | Description | +|---|---|---| +| `error` | string | Always `"plan_quality_error"` | +| `message` | string | `str(PlanQualityError)` — human-readable summary | +| `defects` | list | One entry per failing/warning check | +| `defects[].code` | string | Machine-readable defect code (e.g. `review_missing`, `audit_missing`, `empty_plan`) | +| `defects[].severity` | string | `critical`, `warning`, or `info` | +| `defects[].message` | string | Full defect message, including the `Remediation:` sentence | +| `defects[].remediation` | string | The remediation text extracted from `message` (empty string if the message had no `Remediation:` marker) | + ### Common HTTP Status Codes | Status | Meaning | When returned | @@ -2723,7 +2762,7 @@ For validation errors (422), the response includes field-level details: | `403` | Forbidden | Policy violation (e.g. self-approval rejected in team approval mode) | | `404` | Not Found | Resource does not exist | | `409` | Conflict | Resource state mismatch (e.g. task not awaiting approval, phase_id mismatch) | -| `422` | Unprocessable Entity | Pydantic validation failure (automatic from FastAPI) | +| `422` | Unprocessable Entity | Pydantic validation failure (automatic from FastAPI), or a Forge-generated plan failing the quality gate (see [Plan Quality Errors](#plan-quality-errors-422)) | | `500` | Internal Server Error | Unexpected server-side failure or data integrity error | | `503` | Service Unavailable | Dependent subsystem unavailable (e.g. compliance audit write failure) | diff --git a/docs/cli-reference.md b/docs/cli-reference.md index f9e0fe44..230ce909 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -29,6 +29,7 @@ Commands are organized into functional groups: | **Query** | Project-local structured queries | `query` | | **Cross-Project Query** | Cross-project SQL against central.db | `cquery` | | **Source** | External work-item connections | `source add`, `source list`, `source sync`, `source remove`, `source map` | +| **Diagnostics** | Installation and workspace health checks | `doctor` | | **API** | HTTP API server | `serve` | --- @@ -1877,8 +1878,22 @@ baton knowledge SUBCOMMAND [options] Validate knowledge packs and print actionable warnings (missing or invalid `knowledge.yaml`, declared documents that don't exist, empty descriptions, duplicate document names, documents too large for inline -delivery). Runtime loading tolerates these issues; doctor reports them -as actionable edits without changing loading semantics. +delivery, or a pack/root directory that can't be listed or read). Runtime +loading tolerates these issues; doctor reports them as actionable edits +without changing loading semantics. + +A declared document with a known non-Markdown extension (`.json`, +`.yaml`, `.yml`, `.txt`, `.py`, `.toml`, `.csv`) must exist under that +exact name — it is not satisfied by a same-named `.md` shadow file (e.g. +a manifest declaring `config.json` is not satisfied by `config.json.md`). +Only genuinely extensionless declared names fall back to a `.md` suffix. + +A pack directory (or the root itself) that raises an OS-level error when +listed or read is reported as an `unreadable-pack` issue and skipped, so +one bad directory (permissions, a cloud-sync placeholder, etc.) doesn't +abort the rest of the run. Repeated `--knowledge-root` arguments that +resolve to the same (non-existent) directory now report a single +`missing-root` issue instead of one per repetition. ``` baton knowledge doctor [--knowledge-root DIR ...] [--format text|json] [--json] [--strict] @@ -1886,7 +1901,7 @@ baton knowledge doctor [--knowledge-root DIR ...] [--format text|json] [--json] | Flag | Default | Description | |------|---------|-------------| -| `--knowledge-root DIR` | global + project | Knowledge root to validate; repeatable (default: `~/.claude/knowledge` and `./.claude/knowledge`) | +| `--knowledge-root DIR` | global + project | Knowledge root to validate; repeatable and de-duplicated after resolving to an absolute path (default: `~/.claude/knowledge` and `./.claude/knowledge`) | | `--format FORMAT` | `text` | Output format: `text` or `json` | | `--json` | -- | Alias for `--format json` | | `--strict` | false | Exit non-zero when any warning is found | @@ -2745,6 +2760,51 @@ baton source map ado-contoso-platform 12345 nds task-abc123 --type implements --- +## Diagnostics Commands + +### `baton doctor` + +Read-only installation and workspace health report: Python and package +versions, bundled and project agents, knowledge packs, assurance packs, +PMO UI assets, package resources, optional CLIs (`bd`, `claude`), the +`.beads` workspace, git and git-worktree status, `.claude/team-context` +writability, saved-plan (planner) validation, and terminology. Makes no +changes to the project. + +``` +baton doctor [--json] +``` + +| Flag | Default | Description | +|------|---------|-------------| +| `--json` | false | Emit the report as JSON (schema-versioned) instead of the human-readable summary | + +Exits non-zero (`1`) if any check reports `error` status. + +#### Planner validation check + +The `planner_validation` check validates the most relevant saved +`plan.json` with the same rules as `baton plan-validate`. Doctor picks which +plan to validate by resolving an active task the same way the rest of +the CLI does — `BATON_TASK_ID` env var, then the active-task marker in +the project's SQLite store (honoring `BATON_DB_PATH` if set), then the +`.claude/team-context/active-task-id.txt` file marker — and reports +which path it took in the check's `details.plan_selection` field: + +| `plan_selection` | Meaning | +|------------------|---------| +| `active-task` | An active task resolved; its `plan.json` was validated | +| `fallback-first-found` | No active task resolved; doctor validated the first saved plan found, in fixed order: `.claude/team-context/plan.json`, project-root `plan.json`, then each `executions/*/plan.json` (sorted) | + +When `plan_selection` is `fallback-first-found` and a plan was actually +found, the human-readable message adds a caveat: `(caveat: no active +task resolved; validating the first saved plan found, not necessarily +the current one)`. No caveat is added when no plan was found at all — +that case is reported separately as `No saved plan is available to +validate`, not a guess. + +--- + ## API Server ### `baton serve` diff --git a/docs/internal/doc-audit.md b/docs/internal/doc-audit.md index 62745152..8dfe0ae8 100644 --- a/docs/internal/doc-audit.md +++ b/docs/internal/doc-audit.md @@ -545,3 +545,37 @@ Code lives under `agent_baton/core/manager/`, `agent_baton/core/config/manager.p `agent_baton/cli/commands/knowledge/pack_cmds.py`. Tests live under `tests/manager/`, `tests/cli/`, `tests/e2e/` (see `docs/internal/manager-mode-pmo-design.md` "Testing"). + +--- + +## 2026-07-06 — Bug-fix wave doc sync: signal-forge 422 + doctor `plan_selection` + knowledge-doctor hardening (bd-ftd) + +Captured in: + +- **Public**: + - `docs/api-reference.md` — `422` `plan_quality_error` row added to the + Error Responses tables for `POST /pmo/forge/plan`, + `POST /pmo/forge/regenerate`, and `POST /pmo/signals/{signal_id}/forge`; + new §8 "Plan Quality Errors (422)" subsection documenting the + `plan_quality_error_detail()` response shape (`error`/`message`/`defects`). + - `docs/cli-reference.md` — new "Diagnostics Commands" / `baton doctor` + section (the top-level command had no prior entry) covering the + `planner_validation` check's `plan_selection` field + (`active-task` vs. `fallback-first-found`), the fallback caveat text, + and the `BATON_DB_PATH`-aware active-task probe; `baton knowledge + doctor` section updated for the `unreadable-pack` issue code, the + tightened `_KNOWN_NON_MD_EXTENSIONS` fallback (a declared `config.json` + is no longer satisfied by `config.json.md`), and de-duplicated + `--knowledge-root` missing-root reporting. + +No internal design doc accompanied this pass — it is a documentation-only +sync for bug fixes already merged in commits `3e67452`, `1bf0cfc`, +`129bac4`, `30283b5`, `e674e5d`, `c6ee617`, and `5dc5cc2` (bd-c40, bd-6cr, +bd-j9f). + +Code lives in `agent_baton/api/routes/pmo.py` (`forge_plan`, +`forge_regenerate`, `forge_signal`) + `agent_baton/api/planner_errors.py`, +and `agent_baton/cli/commands/diagnostics_cmd.py` + +`agent_baton/cli/commands/knowledge/doctor_cmd.py`. Tests live in +`tests/test_api_pmo.py`, `tests/cli/test_doctor.py`, and +`tests/knowledge/test_knowledge_doctor.py`. diff --git a/pmo-ui/e2e/utils/audit-reporter.ts b/pmo-ui/e2e/utils/audit-reporter.ts index c6d03a16..680d0048 100644 --- a/pmo-ui/e2e/utils/audit-reporter.ts +++ b/pmo-ui/e2e/utils/audit-reporter.ts @@ -19,6 +19,7 @@ /// import * as fs from 'node:fs'; import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; export type TestStatus = 'pass' | 'fail' | 'skip'; @@ -52,8 +53,10 @@ export interface AuditSummary { // --------------------------------------------------------------------------- +// fileURLToPath (not URL.pathname): on Windows, pathname yields "/C:/…", +// which path.resolve mangles into "C:\C:\…". const REPORT_DIR = path.resolve( - new URL('..', import.meta.url).pathname, + fileURLToPath(new URL('..', import.meta.url)), 'reports', ); diff --git a/pmo-ui/e2e/utils/screenshots.ts b/pmo-ui/e2e/utils/screenshots.ts index f1505c3b..9ae56a9c 100644 --- a/pmo-ui/e2e/utils/screenshots.ts +++ b/pmo-ui/e2e/utils/screenshots.ts @@ -16,9 +16,12 @@ import type { Page, Locator } from '@playwright/test'; import * as path from 'node:path'; import * as fs from 'node:fs'; +import { fileURLToPath } from 'node:url'; +// fileURLToPath (not URL.pathname): on Windows, pathname yields "/C:/…", +// which path.resolve mangles into "C:\C:\…". const SCREENSHOT_DIR = path.resolve( - new URL('..', import.meta.url).pathname, + fileURLToPath(new URL('..', import.meta.url)), 'screenshots', ); diff --git a/pmo-ui/src/components/SpecsPanel.tsx b/pmo-ui/src/components/SpecsPanel.tsx index ede77663..c2536e2e 100644 --- a/pmo-ui/src/components/SpecsPanel.tsx +++ b/pmo-ui/src/components/SpecsPanel.tsx @@ -603,7 +603,11 @@ export function SpecsPanel({ onBack }: SpecsPanelProps) { if (stateFilter) params.state = stateFilter; if (taskTypeFilter) params.task_type = taskTypeFilter; const res = await api.listSpecs(params); - setSpecs(res.specs); + // GET /api/v1/pmo/specs is owned by the spec-queue router and returns + // a bare SpecDraftResponse[] — not the {specs: [...]} SpecListResponse + // this panel was written against. Coerce defensively: an undefined + // deref here crashes the whole app at mount (no error boundary above). + setSpecs(Array.isArray(res) ? [] : res?.specs ?? []); } catch (e) { setFetchError(e instanceof Error ? e.message : 'Failed to load specs'); } finally { diff --git a/pmo-ui/src/components/__tests__/SpecsPanel.test.tsx b/pmo-ui/src/components/__tests__/SpecsPanel.test.tsx new file mode 100644 index 00000000..e52cb09a --- /dev/null +++ b/pmo-ui/src/components/__tests__/SpecsPanel.test.tsx @@ -0,0 +1,42 @@ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { SpecsPanel } from '../SpecsPanel'; +import { api } from '../../api/client'; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('SpecsPanel', () => { + it('survives a bare-array response from GET /pmo/specs (spec-queue shape)', async () => { + // The spec-queue router owns GET /api/v1/pmo/specs and returns a bare + // list[SpecDraftResponse]; this panel was written against a + // {specs: [...]} envelope. Regression for the shape mismatch that + // crashed the whole app at mount (specs became undefined, then + // specs.find threw during render with no error boundary above). + vi.spyOn(api, 'listSpecs').mockResolvedValue([] as never); + + render( {}} />); + + expect(await screen.findByText(/0 specs/)).toBeInTheDocument(); + }); + + it('renders specs from the {specs: [...]} envelope shape', async () => { + vi.spyOn(api, 'listSpecs').mockResolvedValue({ + specs: [ + { + spec_id: 'spec-1', + title: 'sample spec', + state: 'draft', + task_type: 'feature', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + }, + ], + } as never); + + render( {}} />); + + expect(await screen.findByText(/1 spec\b/)).toBeInTheDocument(); + }); +}); diff --git a/tests/cli/test_doctor.py b/tests/cli/test_doctor.py index 77c9f27c..6768dada 100644 --- a/tests/cli/test_doctor.py +++ b/tests/cli/test_doctor.py @@ -835,6 +835,57 @@ def test_doctor_prefers_baton_task_id_env_over_sqlite_active_task( assert check["details"]["plan_path"] == str(env_task_dir / "plan.json") +def test_doctor_reads_active_task_from_baton_db_path_override( + tmp_path: Path, + monkeypatch, +) -> None: + """BATON_DB_PATH must be honoured by the sqlite active-task probe (D1).""" + from agent_baton.cli.commands import diagnostics_cmd + + home = tmp_path / "home" + home.mkdir() + team_context = tmp_path / ".claude" / "team-context" + task_dir = team_context / "executions" / "task-override" + task_dir.mkdir(parents=True) + (task_dir / "plan.json").write_text( + json.dumps(_valid_saved_plan("Task override plan")), + encoding="utf-8", + ) + + # The overridden DB lives entirely outside team-context/baton.db so the + # default path would never find it. + override_db_dir = tmp_path / "elsewhere" + override_db_dir.mkdir(parents=True) + override_db_path = override_db_dir / "custom-baton.db" + conn = sqlite3.connect(override_db_path) + conn.execute( + "CREATE TABLE active_task (id INTEGER PRIMARY KEY, task_id TEXT)" + ) + conn.execute( + "INSERT INTO active_task (id, task_id) VALUES (1, 'task-override')" + ) + conn.commit() + conn.close() + + # Confirm there is no baton.db at the default location doctor would + # otherwise fall back to. + assert not (team_context / "baton.db").exists() + + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + monkeypatch.setenv("PATH", "") + monkeypatch.setenv("BATON_DB_PATH", str(override_db_path)) + + payload = diagnostics_cmd.build_report(tmp_path) + check = _check(payload, "planner_validation") + + assert check["status"] == "ok" + assert check["details"]["active_task_id"] == "task-override" + assert check["details"]["active_task_source"] == "sqlite" + assert check["details"]["plan_path"] == str(task_dir / "plan.json") + + def test_doctor_prefers_active_task_scoped_plan_over_sorted_or_legacy_fallbacks( tmp_path: Path, monkeypatch, @@ -881,6 +932,47 @@ def test_doctor_prefers_active_task_scoped_plan_over_sorted_or_legacy_fallbacks( ] +def test_doctor_flags_fallback_plan_selection_with_no_active_task( + tmp_path: Path, + monkeypatch, +) -> None: + """No active task resolves anywhere -> the fallback guess is surfaced (D2).""" + from agent_baton.cli.commands import diagnostics_cmd + + home = tmp_path / "home" + home.mkdir() + team_context = tmp_path / ".claude" / "team-context" + task_a_dir = team_context / "executions" / "task-a" + task_b_dir = team_context / "executions" / "task-b" + task_a_dir.mkdir(parents=True) + task_b_dir.mkdir(parents=True) + (task_a_dir / "plan.json").write_text( + json.dumps(_valid_saved_plan("Task A plan")), + encoding="utf-8", + ) + (task_b_dir / "plan.json").write_text( + json.dumps(_valid_saved_plan("Task B plan")), + encoding="utf-8", + ) + # No BATON_TASK_ID, no active_task sqlite row/db, no active-task-id.txt + # marker -> nothing identifies a current task. + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("HOME", str(home)) + monkeypatch.setenv("USERPROFILE", str(home)) + monkeypatch.setenv("PATH", "") + monkeypatch.delenv("BATON_TASK_ID", raising=False) + monkeypatch.delenv("BATON_DB_PATH", raising=False) + + payload = diagnostics_cmd.build_report(tmp_path) + check = _check(payload, "planner_validation") + + assert check["details"]["active_task_id"] is None + assert check["details"]["plan_selection"] == "fallback-first-found" + assert check["details"]["plan_path"] == str(task_a_dir / "plan.json") + assert "caveat" in check["message"].lower() + assert "no active task resolved" in check["message"].lower() + + def test_doctor_reports_missing_active_task_plan_without_validating_fallback( tmp_path: Path, monkeypatch, @@ -911,6 +1003,7 @@ def test_doctor_reports_missing_active_task_plan_without_validating_fallback( assert check["status"] == "warning" assert check["details"]["active_task_id"] == "task-b" assert check["details"]["active_task_source"] == "file" + assert check["details"]["plan_selection"] == "active-task" assert check["details"]["plan_path"] == str(missing_active_plan) assert str(missing_active_plan) in check["message"] assert check["details"]["plan_candidates"] == [ diff --git a/tests/cli/test_execute_run_resume.py b/tests/cli/test_execute_run_resume.py index 50ff21c3..a82cef66 100644 --- a/tests/cli/test_execute_run_resume.py +++ b/tests/cli/test_execute_run_resume.py @@ -40,7 +40,10 @@ ExecutionState, GateResult, MachinePlan, + PlanPhase, + PlanStep, StepResult, + TeamMember, ) @@ -724,3 +727,81 @@ def test_resume_defaults_when_flags_omitted(self) -> None: ns = root.parse_args(["execute", "resume"]) assert getattr(ns, "abort", None) is False assert getattr(ns, "no_rerun_gate", None) is False + + +def _team_resume_plan() -> MachinePlan: + """Single-phase plan whose only step is a team step, so resume() walks + straight into _team_dispatch_action (which selects the team backend).""" + return MachinePlan( + task_id="team-resume-strict", + task_summary="team resume strict backend", + phases=[PlanPhase( + phase_id=1, name="Build", + steps=[PlanStep( + step_id="1.1", agent_name="team", + task_description="implement and review", model="sonnet", + team=[ + TeamMember( + member_id="1.1.a", agent_name="backend-engineer", + role="implementer", task_description="impl", + model="sonnet", + ), + TeamMember( + member_id="1.1.b", agent_name="code-reviewer", + role="reviewer", task_description="review", + model="sonnet", + ), + ], + )], + )], + ) + + +class TestResumeUnknownStrictBackend: + """C3 regression: `baton execute resume` must translate an + UnknownTeamBackendError (raised through _team_dispatch_action when + BATON_TEAMS_BACKEND is unknown under BATON_TEAMS_BACKEND_STRICT=1) into a + clean, non-zero exit with the API's message shape — never a raw + traceback. The API maps this deliberately (executions.py:93-104); the CLI + previously had no handler. + """ + + def test_resume_unknown_strict_backend_clean_exit( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture, + ) -> None: + monkeypatch.setenv("BATON_TEAMS_BACKEND", "not-a-real-backend") + monkeypatch.setenv("BATON_TEAMS_BACKEND_STRICT", "1") + + plan = _team_resume_plan() + state = ExecutionState( + task_id=plan.task_id, plan=plan, + current_phase=0, status="running", + ) + StatePersistence(tmp_path, task_id=plan.task_id).save(state) + + args = argparse.Namespace( + subcommand="resume", + task_id=plan.task_id, + output="text", + abort=False, + no_rerun_gate=False, + force_override=False, + override_justification="", + ) + patches = _patches_for_run(tmp_path) + with ( + patches[0], patches[1], patches[2], patches[3], + patches[4], patches[5], patches[6], + pytest.raises(SystemExit) as exc_info, + ): + _mod.handler(args) + + assert exc_info.value.code != 0 + captured = capsys.readouterr() + out = captured.out + captured.err + assert "Unknown BATON_TEAMS_BACKEND" in out + # No traceback leaked to the user. + assert "Traceback" not in out diff --git a/tests/engine/planning/test_decomposition_fanout.py b/tests/engine/planning/test_decomposition_fanout.py index 170a85af..076e897f 100644 --- a/tests/engine/planning/test_decomposition_fanout.py +++ b/tests/engine/planning/test_decomposition_fanout.py @@ -389,3 +389,57 @@ def test_no_research_concerns_leaves_draft_concerns_empty(self) -> None: assert draft.concerns == [], ( "draft.concerns must stay empty when research_concerns is None" ) + + +# --------------------------------------------------------------------------- +# Test 6 — Regression (bd-pz4 part 2): explicit complexity override must +# pair the roster with the phase list it actually built +# --------------------------------------------------------------------------- + +class TestComplexityOverrideRosterPhasePairing: + """``create_plan(complexity="medium")`` takes the explicit-override + path: phases come from ``KeywordClassifier._select_phases()`` (which + drops "Review" at medium complexity), while the roster comes from the + static ``rules/default_agents.py`` ``DEFAULT_AGENTS`` table (which + unconditionally includes ``code-reviewer`` for e.g. "new-feature"). + + Without pairing the two, a rostered ``code-reviewer`` with no Review + phase to live in gets folded into the Implement phase instead, and + ``ValidationStage``'s ``review_missing`` hard gate then rejects the + plan with ``PlanQualityError``. + """ + + def test_medium_override_with_default_reviewer_builds_without_review_missing( + self, + ) -> None: + from agent_baton.core.engine.planner import IntelligentPlanner + + # No PlanQualityError should be raised on this path. + plan = IntelligentPlanner().create_plan( + "Add a reporting endpoint with tests and docs", + task_type="new-feature", + complexity="medium", + ) + + # KeywordClassifier._select_phases drops "Review" at medium + # complexity for "new-feature" -- confirm no Review phase exists. + phase_names = { + p.name.lower().split(":")[0].strip() for p in plan.phases + } + assert "review" not in phase_names, ( + "medium-complexity 'new-feature' plans should have no Review " + "phase (KeywordClassifier drops it at this tier)" + ) + + # ...and therefore code-reviewer (rostered unconditionally by + # DEFAULT_AGENTS["new-feature"]) must have been dropped from the + # roster rather than stranded inside the Implement phase. + all_agents = { + step.agent_name.split("--")[0] + for phase in plan.phases + for step in phase.steps + } + assert "code-reviewer" not in all_agents, ( + "code-reviewer must be dropped from the roster when the " + "complexity-override phase list has no Review phase to hold it" + ) diff --git a/tests/engine/planning/test_planner_review.py b/tests/engine/planning/test_planner_review.py index 8f774b3e..a9659706 100644 --- a/tests/engine/planning/test_planner_review.py +++ b/tests/engine/planning/test_planner_review.py @@ -4,6 +4,17 @@ 1. Flag unset → HeadlessClaude is never constructed. 2. Flag set to a recognised model alias → review runs with the correct model. 3. Flag set to an unrecognised value → warning logged, HeadlessClaude not constructed. + +These tests target ``IntelligentPlanner._review_plan_with_llm`` in isolation +from ``FallbackClassifier``'s *separate* ``TalentAgentClassifier`` -> +``HeadlessClaude`` probe that the classification stage runs unconditionally +(regardless of ``BATON_PLAN_REVIEW``) to decide between LLM-backed and +keyword-heuristic classification. The planner's ``task_classifier`` is +pinned to the deterministic ``KeywordClassifier`` -- the same pattern used +by ``tests/e2e/test_manager_mode_planning.py`` -- so the single +``HeadlessClaude`` mock installed in each test below observes only the +post-pipeline review call under test, not the unrelated classification +probe. """ from __future__ import annotations @@ -15,9 +26,17 @@ @pytest.fixture def planner(): - """Build a fresh IntelligentPlanner for each test.""" + """Build a fresh IntelligentPlanner for each test. + + ``task_classifier=KeywordClassifier()`` bypasses ``FallbackClassifier``'s + default ``TalentAgentClassifier``, which would otherwise construct its + own ``HeadlessClaude`` instance during classification and confound the + ``HeadlessClaude`` mocks these tests install to observe the (unrelated) + ``BATON_PLAN_REVIEW`` review step. + """ + from agent_baton.core.engine.classifier import KeywordClassifier from agent_baton.core.engine.planning.planner import IntelligentPlanner - return IntelligentPlanner() + return IntelligentPlanner(task_classifier=KeywordClassifier()) class TestPlanReview: diff --git a/tests/engine/planning/test_planner_smoke.py b/tests/engine/planning/test_planner_smoke.py index 9082958b..ae8f30c5 100644 --- a/tests/engine/planning/test_planner_smoke.py +++ b/tests/engine/planning/test_planner_smoke.py @@ -1096,6 +1096,39 @@ def test_risk_none_is_a_safe_no_op(self): assert "code-reviewer" not in draft.resolved_agents + def test_medium_risk_reviewer_already_present_gets_review_phase_slot(self): + """bd-pz4 regression. + + A MEDIUM-risk task whose roster ALREADY carries ``code-reviewer`` + (so ``_ensure_safety_roster`` injects nothing) must still get a + "Review" slot appended to ``classified_phases``. Before the fix, + the classified_phases guarantee only fired when THIS call injected + a reviewer/auditor (``injected_any``) -- an already-present reviewer + left ``classified_phases`` untouched, so the reviewer was + force-assigned into the Implement team-step and then filtered out + by ``consolidate_team_step`` (phase_builder.py), leaving zero + review coverage and tripping ValidationStage's ``review_missing`` + gate (which blocks on roster membership alone, at any risk level). + """ + from agent_baton.core.engine.planning.stages.risk import RiskStage + from agent_baton.models.enums import RiskLevel + + draft = self._make_draft( + "Migrate the user table to PostgreSQL", + RiskLevel.MEDIUM, + resolved_agents=["architect", "backend-engineer", "code-reviewer"], + ) + draft.classified_phases = ["Design", "Implement", "Test"] + + RiskStage()._ensure_safety_roster(draft) + + # No injection should have occurred -- code-reviewer was already there. + assert draft.resolved_agents.count("code-reviewer") == 1 + assert not any("injected code-reviewer" in note for note in draft.routing_notes) + # But a Review phase slot must now exist so the pre-existing reviewer + # has somewhere to land. + assert "Review" in draft.classified_phases + # --- Full-pipeline integration (uses IntelligentPlanner) --- def test_high_risk_plan_has_code_reviewer_in_plan(self, plan_for): diff --git a/tests/engine/test_team_backends.py b/tests/engine/test_team_backends.py index 70af5931..2e75ecfb 100644 --- a/tests/engine/test_team_backends.py +++ b/tests/engine/test_team_backends.py @@ -258,6 +258,37 @@ def test_audit_missing_dir_returns_empty(self, tmp_path: Path) -> None: flagged = audit_agents_for_teammate_safety(tmp_path / "nope") assert flagged == {} + def test_step_audit_honors_first_existing_dir_over_stale_sibling( + self, tmp_path: Path, + ) -> None: + """C2 regression: an authoritative clean .claude/agents must not be + overridden by a stale divergent repo-level agents/ copy. The step + audit resolves against the FIRST EXISTING directory, so a clean + higher-priority dir wins even when a lower-priority sibling flags. + """ + team_context_root = tmp_path / ".claude" / "team-context" + team_context_root.mkdir(parents=True) + + # Authoritative, clean .claude/agents (first candidate dir). + claude_agents = tmp_path / ".claude" / "agents" + claude_agents.mkdir() + (claude_agents / "backend-engineer.md").write_text( + "---\nname: backend-engineer\nmodel: sonnet\n---\nbody\n", + encoding="utf-8", + ) + + # Stale divergent repo-level agents/ copy that flags the same agent. + repo_agents = tmp_path / "agents" + repo_agents.mkdir() + (repo_agents / "backend-engineer.md").write_text( + "---\nname: backend-engineer\nmodel: sonnet\nskills: foo\n---\n", + encoding="utf-8", + ) + + step = _plan_with_team().phases[0].steps[0] + flagged = ClaudeTeamsBackend._audit_step_agents(step, team_context_root) + assert flagged == {} + class TestTeamReadinessDiagnostics: def test_worktree_diagnostics_summarize_team_shape(self, tmp_path: Path) -> None: diff --git a/tests/engine/test_team_mailbox_hooks.py b/tests/engine/test_team_mailbox_hooks.py index a88a19ab..96e0d60b 100644 --- a/tests/engine/test_team_mailbox_hooks.py +++ b/tests/engine/test_team_mailbox_hooks.py @@ -192,6 +192,47 @@ def test_next_actions_persists_team_readiness_for_unlocked_team( == "worktree" ) + def test_resume_first_entry_persists_team_readiness( + self, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """C1 regression: when resume() is the FIRST walk into a team step, + the team_readiness diagnostic mutated by _team_dispatch_action must be + persisted. resume() previously returned without saving, so the + once-per-step diagnostic (gated on the parent StepResult being absent) + was lost after a crash/resume boundary and never recomputed once + team-record created the parent StepResult. + """ + monkeypatch.delenv("BATON_TEAMS_BACKEND", raising=False) + monkeypatch.delenv("BATON_TEAMS_BACKEND_STRICT", raising=False) + engine = ExecutionEngine(team_context_root=tmp_path) + engine.start(_solo_then_team_plan()) + engine.record_step_result( + step_id="1.1", + agent_name="setup-agent", + status="complete", + outcome="inputs ready", + ) + + # Persisted state now has no team_readiness for the still-locked 1.2. + pre = StatePersistence(tmp_path).load() + assert pre is not None + assert "1.2" not in pre.plan.plan_diagnostics.get("team_readiness", {}) + + # Simulate crash/resume: reconstruct a fresh engine and let resume() + # perform the first walk into the team step. + resumed = ExecutionEngine(team_context_root=tmp_path) + action = resumed.resume() + assert action is not None + + state = StatePersistence(tmp_path).load() + assert state is not None + assert ( + state.plan.plan_diagnostics["team_readiness"]["1.2"]["backend"] + == "worktree" + ) + class TestMailboxOnMemberResult: def test_task_completed_event_on_success(self, tmp_path: Path) -> None: diff --git a/tests/knowledge/test_knowledge_doctor.py b/tests/knowledge/test_knowledge_doctor.py index c434442e..9e20028d 100644 --- a/tests/knowledge/test_knowledge_doctor.py +++ b/tests/knowledge/test_knowledge_doctor.py @@ -340,6 +340,119 @@ def test_declared_doc_with_dotted_stem_resolves_md_fallback( ) +def test_declared_json_doc_not_satisfied_by_md_shadow( + tmp_path: Path, monkeypatch, capsys +) -> None: + _isolate_defaults(monkeypatch, tmp_path) + pack = tmp_path / ".claude" / "knowledge" / "shadowed" + pack.mkdir(parents=True) + (pack / "knowledge.yaml").write_text( + yaml.safe_dump( + { + "name": "shadowed", + "description": "Shadowed declaration fixtures", + "documents": ["config.json"], + }, + sort_keys=False, + ), + encoding="utf-8", + ) + # A shadow "config.json.md" must NOT satisfy the "config.json" declaration + # — only genuinely extensionless stems get the +.md fallback. + (pack / "config.json.md").write_text("shadow", encoding="utf-8") + + rc = _run_cli(["knowledge", "doctor", "--json"]) + data = json.loads(capsys.readouterr().out) + missing = [ + issue + for issue in data["issues"] + if issue["code"] == "missing-declared-file" + ] + + assert rc == 0 + assert len(missing) == 1 + assert "config.json" in missing[0]["message"] + + +def test_doctor_isolates_unreadable_pack_and_continues( + tmp_path: Path, monkeypatch, capsys +) -> None: + _isolate_defaults(monkeypatch, tmp_path) + knowledge = tmp_path / ".claude" / "knowledge" + + good_pack = knowledge / "good-pack" + good_pack.mkdir(parents=True) + (good_pack / "knowledge.yaml").write_text( + yaml.safe_dump( + { + "name": "good-pack", + "description": "Good pack", + "default_delivery": "reference", + }, + sort_keys=False, + ), + encoding="utf-8", + ) + _write_doc(good_pack / "guide.md", name="guide", description="Valid guide") + + locked_pack = knowledge / "locked-pack" + locked_pack.mkdir(parents=True) + (locked_pack / "knowledge.yaml").write_text( + yaml.safe_dump( + {"name": "locked-pack", "description": "Locked pack"}, + sort_keys=False, + ), + encoding="utf-8", + ) + + original_glob = Path.glob + + def flaky_glob(self: Path, pattern: str): + if self == locked_pack and pattern == "*.md": + raise PermissionError("simulated ACL denial") + return original_glob(self, pattern) + + monkeypatch.setattr(Path, "glob", flaky_glob) + + rc = _run_cli(["knowledge", "doctor", "--json"]) + data = json.loads(capsys.readouterr().out) + + assert rc == 0 + unreadable = [ + issue for issue in data["issues"] if issue["code"] == "unreadable-pack" + ] + assert len(unreadable) == 1 + assert unreadable[0]["pack"] == "locked-pack" + # The good pack was still validated despite the locked pack's failure. + assert data["summary"]["packs"] == 2 + assert data["summary"]["documents"] == 1 + + +def test_strict_duplicate_explicit_root_emits_single_issue( + tmp_path: Path, monkeypatch, capsys +) -> None: + _isolate_defaults(monkeypatch, tmp_path) + missing_root = tmp_path / "does-not-exist" + + rc = _run_cli([ + "knowledge", + "doctor", + "--json", + "--knowledge-root", + str(missing_root), + "--knowledge-root", + str(missing_root), + "--strict", + ]) + data = json.loads(capsys.readouterr().out) + missing = [ + issue for issue in data["issues"] if issue["code"] == "missing-root" + ] + + assert rc == 1 + assert len(missing) == 1 + + def test_strict_missing_explicit_root_emits_issue_and_fails( tmp_path: Path, monkeypatch, capsys ) -> None: diff --git a/tests/models/test_execution_sqlite_roundtrip.py b/tests/models/test_execution_sqlite_roundtrip.py index e178a90f..e7867e70 100644 --- a/tests/models/test_execution_sqlite_roundtrip.py +++ b/tests/models/test_execution_sqlite_roundtrip.py @@ -440,6 +440,47 @@ def test_plan_sqlite_upsert_is_idempotent(self, store: SqliteStorage) -> None: assert loaded is not None assert loaded.task_id == plan.task_id + def test_plan_upsert_preserves_release_id_on_resave( + self, store: SqliteStorage + ) -> None: + """B1 regression: re-saving a plan must not wipe release_id. + + ``_upsert_plan`` previously used ``INSERT OR REPLACE`` against a + table whose PK is ``task_id`` -- SQLite implements that as + DELETE+INSERT, and the child columns not listed in the statement + (release_id, which release_store.tag_plan sets separately) were + reset to NULL. Every subsequent ``save_execution`` call would then + silently un-tag the plan's release. + """ + from agent_baton.core.storage.release_store import ReleaseStore + from agent_baton.models.release import Release + + plan = _minimal_plan("task-plan-rt-007") + store.save_plan(plan) + + release_store = ReleaseStore(store.db_path) + release_store.create( + Release( + release_id="rel-001", + name="Q1 release", + target_date="2026-03-01", + status="planned", + ) + ) + assert release_store.tag_plan("task-plan-rt-007", "rel-001") is True + + # Re-save the plan (simulates save_execution calling _upsert_plan + # again on a later state transition) -- release_id must survive. + store.save_plan(plan) + + conn = store._conn() # noqa: SLF001 - direct check of raw column + row = conn.execute( + "SELECT release_id FROM plans WHERE task_id = ?", + ("task-plan-rt-007",), + ).fetchone() + assert row is not None + assert row["release_id"] == "rel-001" + # --------------------------------------------------------------------------- # ExecutionState SQLite roundtrip diff --git a/tests/planning/test_plan_quality_validation.py b/tests/planning/test_plan_quality_validation.py index 19d16fd0..f6dcc9d3 100644 --- a/tests/planning/test_plan_quality_validation.py +++ b/tests/planning/test_plan_quality_validation.py @@ -9,14 +9,25 @@ from agent_baton.core.engine.planner import IntelligentPlanner from agent_baton.core.engine.planning.draft import PlanDraft from agent_baton.core.engine.planning.services import PlannerServices +from agent_baton.core.engine.planning.stages.risk import RiskStage from agent_baton.core.engine.planning.stages.validation import ( PlanDefect, PlanQualityError, ValidationStage, + validate_assembled_plan, +) +from agent_baton.core.engine.planning.utils.risk_and_policy import ( + audit_coverage_requirement, + requires_audit_coverage, ) from agent_baton.core.govern.classifier import ClassificationResult, DataClassifier from agent_baton.models.enums import RiskLevel -from agent_baton.models.execution import PlanPhase, PlanStep +from agent_baton.models.execution import ( + MachinePlan, + PlanPhase, + PlanStep, + TeamMember, +) def _stub_services() -> PlannerServices: @@ -326,3 +337,188 @@ def test_reviewer_agent_in_implementation_phase_without_review_is_mismatch( assert "code-reviewer" in message assert "Review" in message assert "Remediation:" in message + + +def _pack_classification(risk: object) -> SimpleNamespace: + """A pack-classified result (guardrail_preset='pack:').""" + return SimpleNamespace( + guardrail_preset="pack:phi-hipaa", + risk_level=risk, + signals_found=["path:phi/"], + explanation="", + confidence="high", + ) + + +class TestA1PackClassifiedAuditCoverage: + """A1: pack-classified regulated tasks must pull in audit coverage. + + Pack classification sets ``guardrail_preset='pack:'`` (not the + ``Regulated Data`` literal), so the exact-match branch missed them and + both RiskStage auditor injection and the ValidationStage audit gate were + bypassed for pack-classified HIGH/CRITICAL tasks. + """ + + def test_pack_high_risk_enum_requires_audit(self) -> None: + reason = audit_coverage_requirement("benign task", _pack_classification(RiskLevel.HIGH)) + assert reason == "guardrail_preset=pack:phi-hipaa" + assert requires_audit_coverage("benign task", _pack_classification(RiskLevel.CRITICAL)) + + def test_pack_risk_as_string_is_coerced(self) -> None: + # _classification_from_plan can hand back a string risk_level. + assert requires_audit_coverage("benign task", _pack_classification("HIGH")) + assert requires_audit_coverage("benign task", _pack_classification("CRITICAL")) + + def test_pack_low_or_medium_does_not_require_audit(self) -> None: + # Risk-gated: only HIGH/CRITICAL pack tasks pull in the auditor. + assert audit_coverage_requirement("benign task", _pack_classification(RiskLevel.MEDIUM)) is None + assert audit_coverage_requirement("benign task", _pack_classification(RiskLevel.LOW)) is None + + def test_riskstage_injects_auditor_for_pack_high(self) -> None: + # Benign summary (no audit keyword) so injection can only come from + # the pack-preset branch, proving RiskStage now covers pack tasks. + draft = _draft_with_phase(task_summary="Adjust widget layout", risk=RiskLevel.HIGH) + draft.resolved_agents = ["backend-engineer"] + draft.classification = _pack_classification(RiskLevel.HIGH) + + RiskStage()._ensure_safety_roster(draft) + + assert "auditor" in draft.resolved_agents + + def test_validation_gate_blocks_pack_plan_without_audit(self) -> None: + draft = _draft_with_phase(task_summary="Adjust widget layout", risk=RiskLevel.HIGH) + draft.resolved_agents = ["backend-engineer"] + draft.classification = _pack_classification(RiskLevel.HIGH) + + defects = ValidationStage()._detect_defects(draft) + + assert any(d.code == "audit_missing" for d in defects) + + +class TestA2NestedTeamCoverage: + """A2: sub_team is unbounded — coverage/mismatch checks must recurse.""" + + @staticmethod + def _depth2_step(deep_agent: str) -> PlanStep: + deep = TeamMember(member_id="m3", agent_name=deep_agent, role="implementer") + mid = TeamMember(member_id="m2", agent_name="backend-engineer", role="lead", sub_team=[deep]) + lead = TeamMember(member_id="m1", agent_name="backend-engineer", role="lead", sub_team=[mid]) + return PlanStep( + step_id="1.1", + agent_name="team", + task_description="Team step with a depth-2 sub-team member.", + team=[lead], + ) + + def test_step_agent_bases_finds_depth2_member(self) -> None: + step = self._depth2_step("auditor") + assert "auditor" in ValidationStage()._step_agent_bases(step) + + def test_depth2_auditor_satisfies_audit_coverage(self) -> None: + # Regulated task requires audit; auditor buried at depth-2 in the + # Audit phase must be found (else a false audit_missing hard-block). + draft = _draft_with_phase(task_summary="Update GDPR compliance export") + draft.resolved_agents = ["backend-engineer", "auditor"] + draft.plan_phases.append( + PlanPhase(phase_id=2, name="Audit", steps=[self._depth2_step("auditor")]) + ) + + defects = ValidationStage()._detect_defects(draft) + + assert not any(d.code == "audit_missing" for d in defects) + + def test_depth2_reviewer_in_implement_is_flagged(self) -> None: + # A reviewer buried at depth-2 in an Implement phase must not evade + # the reviewer-in-implement mismatch check. + draft = _draft_with_phase(phase_name="Implement") + draft.plan_phases = [ + PlanPhase(phase_id=1, name="Implement", steps=[self._depth2_step("code-reviewer")]) + ] + + defects = ValidationStage()._detect_defects(draft) + + assert any(d.code == "agent_phase_mismatch" for d in defects) + + +class TestA3HeadlessAuditParity: + """A3: validate_assembled_plan (forge/headless) hard-fails with an + actionable defect instead of auto-remediating — outcome parity with the + interactive RiskStage path is achieved by regenerating, not injecting.""" + + def test_assembled_plan_missing_auditor_raises_actionable_error( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("BATON_DEV_MODE", raising=False) + monkeypatch.delenv("BATON_PLANNER_WARN_ONLY", raising=False) + monkeypatch.setenv("BATON_PLANNER_HARD_GATE", "1") + + plan = MachinePlan( + task_id="task-a3-headless", + task_summary="Update GDPR compliance data export workflow", + risk_level="HIGH", + phases=[ + PlanPhase( + phase_id=1, + name="Implement", + steps=[ + PlanStep( + step_id="1.1", + agent_name="backend-engineer", + task_description="Implement the export change.", + ) + ], + ) + ], + task_type="feature", + complexity="medium", + ) + + with pytest.raises(PlanQualityError) as ei: + validate_assembled_plan(plan, services=_stub_services()) + + audit_defects = [d for d in ei.value.defects if d.code == "audit_missing"] + assert audit_defects, "headless path must surface an audit_missing defect" + message = audit_defects[0].message + # Actionable: names the auditor agent + Audit phase remediation and + # documents that parity is outcome parity (auditor on both paths). + assert "auditor" in message + assert "Audit" in message + assert "parity" in message + assert "Remediation:" in message + + +class TestA4PolicyFailureVisible: + """A4: a policy-engine failure must be visible (logged + surfaced on + score_warnings), not silently swallowed — the silent branch voided the + policy-driven audit-requirement path.""" + + def test_policy_engine_error_is_recorded_not_swallowed( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + import dataclasses + + class _BoomEngine: + def load_preset(self, name: str) -> object: + raise RuntimeError("boom") + + services = dataclasses.replace(_stub_services(), policy_engine=_BoomEngine()) + draft = _draft_with_phase() + + # Must not raise — planning stays alive. + ValidationStage()._check_scores(draft=draft, services=services) + + assert any("policy_validation_failed" in w for w in draft.score_warnings) + assert any("boom" in w for w in draft.score_warnings) + + +class TestA5DraftPhaseReviewerCheck: + """A5: the documentation archetype's Draft phase is an implement phase — + reviewer-class agents in it must be flagged.""" + + def test_reviewer_in_draft_phase_is_mismatch(self) -> None: + draft = _draft_with_phase(phase_name="Draft", agent_name="code-reviewer") + draft.resolved_agents = ["code-reviewer"] + + defects = ValidationStage()._detect_defects(draft) + + assert any(d.code == "agent_phase_mismatch" for d in defects) diff --git a/tests/snapshots/plans/compliance-audit.json b/tests/snapshots/plans/compliance-audit.json index 0af9d9ae..f0d77e3c 100644 --- a/tests/snapshots/plans/compliance-audit.json +++ b/tests/snapshots/plans/compliance-audit.json @@ -29,17 +29,11 @@ "phase_id": 2, "steps": [ { - "agent": "team", + "agent": "backend-engineer", "depends_on": [], "step_id": "2.1", "step_type": "developing", - "team": [ - { - "agent": "backend-engineer", - "member_id": "2.1.a", - "role": "lead" - } - ] + "team": [] } ] }, diff --git a/tests/test_api_pmo.py b/tests/test_api_pmo.py index 9c526e26..b824dd20 100644 --- a/tests/test_api_pmo.py +++ b/tests/test_api_pmo.py @@ -1036,6 +1036,26 @@ def test_forge_signal_project_id_only_returns_signal_id(self, client: TestClient ).json() assert body["signal_id"] == "fsig8-001" + def test_plan_quality_error_returns_structured_422( + self, client: TestClient, app + ) -> None: + _register_project(client, project_id="fsig9-proj", program="FS9") + _create_signal(client, signal_id="fsig9-001") + app.dependency_overrides[get_forge_session]().signal_to_plan.side_effect = ( + _plan_quality_error() + ) + + r = client.post( + "/api/v1/pmo/signals/fsig9-001/forge", + json={"project_id": "fsig9-proj"}, + ) + + assert r.status_code == 422 + detail = r.json()["detail"] + assert detail["error"] == "plan_quality_error" + assert detail["defects"][0]["code"] == "audit_missing" + assert "Audit phase" in detail["defects"][0]["remediation"] + # =========================================================================== # POST /api/v1/pmo/signals/{signal_id}/resolve — full signal response diff --git a/tests/test_storage_migrate.py b/tests/test_storage_migrate.py index a071631f..84fd3fb3 100644 --- a/tests/test_storage_migrate.py +++ b/tests/test_storage_migrate.py @@ -329,6 +329,59 @@ def test_idempotent_second_run(self, tmp_path: Path) -> None: # Second run should import 0 new executions (INSERT OR IGNORE) assert imported2["executions"] == 0 + def test_migrates_full_plan_column_set(self, tmp_path: Path) -> None: + """B2 regression: the legacy JSON->SQLite migrator must not drop + plan_diagnostics, explicit_knowledge_packs/docs, intervention_level, + task_type, classification_signals/confidence -- these previously + silently defaulted because the INSERT bound only 10 columns. + """ + ctx = tmp_path / "team-context" + ctx.mkdir() + plan = _minimal_plan("task-002") + plan.task_type = "bugfix" + plan.explicit_knowledge_packs = ["python-web"] + plan.explicit_knowledge_docs = ["docs/schema.md"] + plan.intervention_level = "high" + plan.classification_signals = '{"keywords": ["bug"]}' + plan.classification_confidence = 0.87 + plan.plan_diagnostics = {"classification_source": "haiku"} + + state = ExecutionState( + task_id="task-002", + plan=plan, + status="complete", + started_at="2026-03-01T10:00:00Z", + completed_at="2026-03-01T10:30:00Z", + ) + _write_execution(ctx, state) + + StorageMigrator(ctx).migrate() + + conn = sqlite3.connect(str(ctx / "baton.db")) + conn.row_factory = sqlite3.Row + row = conn.execute( + """ + SELECT explicit_knowledge_packs, explicit_knowledge_docs, + intervention_level, task_type, + classification_signals, classification_confidence, + plan_diagnostics + FROM plans WHERE task_id = ? + """, + ("task-002",), + ).fetchone() + conn.close() + + assert row is not None + assert json.loads(row["explicit_knowledge_packs"]) == ["python-web"] + assert json.loads(row["explicit_knowledge_docs"]) == ["docs/schema.md"] + assert row["intervention_level"] == "high" + assert row["task_type"] == "bugfix" + assert row["classification_signals"] == '{"keywords": ["bug"]}' + assert row["classification_confidence"] == pytest.approx(0.87) + assert json.loads(row["plan_diagnostics"]) == { + "classification_source": "haiku" + } + def test_multiple_executions(self, tmp_path: Path) -> None: ctx = tmp_path / "team-context" ctx.mkdir()