[codex] Add decision and thread planning modules#1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several optional modules to KnowledgeOS, including a Decision Graph module for auditable decision summaries, a chat-level Thread Plan Ledger, and a Mission Flow reporting layer with Mermaid diagrams. It also adds support for local external-write policy overlays to handle runtime migrations safely. The review feedback focuses on hardening the implementation in knowledgeos/cli.py against potential crashes and validation bypasses. Specifically, the reviewer identified several places where null values in YAML or NDJSON files could propagate as None or evaluate to the string 'None', bypassing empty checks or causing TypeError and AttributeError exceptions. Additionally, defensive checks were suggested to prevent crashes when local policy sections are omitted or when task lookups return no results.
| external_forbidden_match = matches_any( | ||
| absolute_value, | ||
| local_external_policy["external_forbidden_without_human_gate"], | ||
| ) | ||
| if external_forbidden_match: | ||
| return { | ||
| "decision": "human_gate_required", | ||
| "reason": f"matches external_forbidden_without_human_gate pattern {external_forbidden_match}", | ||
| "path": absolute_value, | ||
| "inside_project": False, | ||
| } | ||
|
|
||
| external_controlled_match = matches_any( | ||
| absolute_value, | ||
| local_external_policy["external_controlled"], | ||
| ) | ||
| if external_controlled_match: | ||
| receipt_match = matches_any( | ||
| absolute_value, | ||
| local_external_policy["external_require_receipt_for"], | ||
| ) | ||
| return { | ||
| "decision": "allow", | ||
| "reason": f"matches external_controlled pattern {external_controlled_match}", | ||
| "path": absolute_value, | ||
| "inside_project": False, | ||
| "receipt_required": bool(receipt_match), | ||
| "receipt_pattern": receipt_match, | ||
| } |
There was a problem hiding this comment.
If the local external write policy file (.knowledgeos-local/write-policy.local.yaml) does not define all sections (e.g., if external_forbidden_without_human_gate or external_require_receipt_for is omitted), direct subscript access will raise a KeyError and crash the command. Using .get(..., []) provides a safe fallback to an empty list.
external_forbidden_match = matches_any(
absolute_value,
local_external_policy.get("external_forbidden_without_human_gate", []),
)
if external_forbidden_match:
return {
"decision": "human_gate_required",
"reason": f"matches external_forbidden_without_human_gate pattern {external_forbidden_match}",
"path": absolute_value,
"inside_project": False,
}
external_controlled_match = matches_any(
absolute_value,
local_external_policy.get("external_controlled", []),
)
if external_controlled_match:
receipt_match = matches_any(
absolute_value,
local_external_policy.get("external_require_receipt_for", []),
)
return {
"decision": "allow",
"reason": f"matches external_controlled pattern {external_controlled_match}",
"path": absolute_value,
"inside_project": False,
"receipt_required": bool(receipt_match),
"receipt_pattern": receipt_match,
}| return { | ||
| "exists": True, | ||
| "strictness": (scalars.get("strictness") or "warn").lower(), | ||
| "downgrade_reason": scalars.get("downgrade_reason", ""), |
There was a problem hiding this comment.
If downgrade_reason is explicitly set to null or left empty in the YAML file, scalars.get("downgrade_reason", "") will return None. This propagates a None value to policy, which can cause issues or bypass validation checks. Using scalars.get("downgrade_reason") or "" ensures it always defaults to an empty string.
| "downgrade_reason": scalars.get("downgrade_reason", ""), | |
| "downgrade_reason": scalars.get("downgrade_reason") or "", |
| for field in ["title", "summary", "reason", "evidence"]: | ||
| if not str(event.get(field, "")).strip(): | ||
| errors.append({"label": f"decision_missing_{field}", "detail": decision_id}) |
There was a problem hiding this comment.
If any of the validated fields are explicitly set to null in the JSON lines file, event.get(field, "") returns None. str(None) evaluates to "None", which is truthy and bypasses the empty check. Using event.get(field) or "" ensures that both missing and null values are correctly treated as empty strings.
| for field in ["title", "summary", "reason", "evidence"]: | |
| if not str(event.get(field, "")).strip(): | |
| errors.append({"label": f"decision_missing_{field}", "detail": decision_id}) | |
| for field in ["title", "summary", "reason", "evidence"]: | |
| if not str(event.get(field) or "").strip(): | |
| errors.append({"label": f"decision_missing_{field}", "detail": decision_id}) |
| errors.append({"label": "invalid_decision_kind", "detail": kind}) | ||
| if status not in DECISION_EVENT_STATUSES: | ||
| errors.append({"label": "invalid_decision_status", "detail": status}) | ||
| if (kind in DECISION_EXPLANATION_REQUIRED_KINDS or status in {"abandoned", "rolled_back", "superseded"}) and not str(event.get("reason", "")).strip(): |
There was a problem hiding this comment.
If reason is explicitly set to null in the JSON lines file, str(event.get("reason", "")) evaluates to "None", which bypasses the empty check. Using event.get("reason") or "" ensures that both missing and null values are correctly treated as empty strings.
| if (kind in DECISION_EXPLANATION_REQUIRED_KINDS or status in {"abandoned", "rolled_back", "superseded"}) and not str(event.get("reason", "")).strip(): | |
| if (kind in DECISION_EXPLANATION_REQUIRED_KINDS or status in {"abandoned", "rolled_back", "superseded"}) and not str(event.get("reason") or "").strip(): |
| errors.append({"label": "decision_missing_explanation", "detail": decision_id}) | ||
| if not decision_event_has_command_event(run_dir, event, task_id, run_id): | ||
| errors.append({"label": "decision_missing_command_event", "detail": decision_id}) | ||
| parent = str(event.get("parent_id", "")).strip() |
There was a problem hiding this comment.
If parent_id is explicitly set to null in the JSON lines file, str(event.get("parent_id", "")) evaluates to "None". This causes "None" to be added to parent_ids, which is then flagged as an orphan parent. Using event.get("parent_id") or "" ensures that null or missing parent IDs are correctly treated as empty strings.
| parent = str(event.get("parent_id", "")).strip() | |
| parent = str(event.get("parent_id") or "").strip() |
| lines.append(f"- Evidence: {event.get('evidence')}") | ||
| lines.extend(["", "## Full Decision Ledger", ""]) | ||
| for event in events: | ||
| options = ", ".join(str(item) for item in event.get("options", []) if str(item).strip()) |
There was a problem hiding this comment.
If options is explicitly set to null in the JSON lines file, event.get("options", []) will return None. Iterating over None will raise a TypeError and crash the rendering command. Using event.get("options") or [] ensures a safe fallback to an empty list.
| options = ", ".join(str(item) for item in event.get("options", []) if str(item).strip()) | |
| options = ", ".join(str(item) for item in (event.get("options") or []) if str(item).strip()) |
| eval_ok = eval_has_passed(run_dir / "eval.md") | ||
| postflight_text = read_text(run_dir / "postflight.md") if (run_dir / "postflight.md").exists() else "" | ||
| synced = sync_status == "SYNC_OK" or "[SYNC_OK]" in postflight_text | ||
| task_title = task.get("title") or run_meta.get("task_title") or task_id |
There was a problem hiding this comment.
If task is None (e.g., if the task is not found in tasks.yaml), calling task.get("title") will raise an AttributeError and crash the command. Adding a defensive check ensures safe fallback behavior.
| task_title = task.get("title") or run_meta.get("task_title") or task_id | |
| task_title = (task.get("title") if task else None) or run_meta.get("task_title") or task_id |
| "task": task, | ||
| "run_dir": run_dir, | ||
| "title": task_title, | ||
| "complexity": task.get("complexity", "medium"), |
There was a problem hiding this comment.
If task is None, calling task.get("complexity") will raise an AttributeError and crash the command. Adding a defensive check ensures safe fallback behavior.
| "complexity": task.get("complexity", "medium"), | |
| "complexity": task.get("complexity", "medium") if task else "medium", |
1 participant
Summary
Bug Fix Evidence
h1_count=3h1_count=2Validation
python3 -B -m py_compile knowledgeos/cli.pymake smokegit diff --checkNotes
HTML remains presentation-only. Markdown, YAML, and NDJSON remain the canonical KnowledgeOS evidence formats.