docs(review): semantic dedup review of 172 directives

docs/reviews/DIRECTIVE_DEDUP_REVIEW_20260703.md

@@ -0,0 +1,230 @@
+# Semantic Duplicate Review — 172 directives
+
+**Date:** 2026-07-03
+**Method:** Read every v1.md (header + first 200 chars of body) and meta.md (source provenance) across 172 directives under `conductor/directives/*/v1.md`. Grouped by claimed rule; evaluated for overlap. The dump file used is in `tests/artifacts/directive_dump.txt` (preserved for audit).
+
+**Scope decision:** This review is **conservative on consolidation**. The user's directive library philosophy (per the harness spec and recent worker reports) explicitly values keeping distinct rules even when they share a domain, because distinct rules apply in different contexts and removing them erases useful guidance. The user has already pre-flagged three keep-both pairs: `tdd_red_green_required` vs `test_must_fail_for_believed_reason`; `verify_before_editing` vs `evidence_before_completion_claims`; `decompose_or_isolate_never_offload` vs `never_inherit_session_history_to_subagent`. I apply the same conservative posture throughout.
+
+**Counts in the report:**
+- 1 true-duplicate pair (recommend consolidation)
+- 6 near-duplicate pairs/clusters (keep both, document distinction)
+- 12 adjacent-rule clusters (no action, just observed)
+- 14 verified-distinct high-confidence pairs (explicitly checked)
+
+---
+
+## True duplicates (recommend consolidation)
+
+### 1. `file_naming_convention` ↔ `large_files_are_fine`
+
+**Status:** TRUE DUPLICATE. The bodies of these two directives are **character-for-character identical** (the same 7-bullet "File Size and Naming Convention (HARD RULE — added 2026-06-11)" block lifted verbatim from `AGENTS.md:62-67`). The only difference is the title:
+
+- `file_naming_convention`: title is "# New src/<thing>.py files may only be created on the user's explicit request"
+- `large_files_are_fine`: title is "# Large files are FINE — file splitting is not a quality metric"
+
+But both bodies contain BOTH the "Large files are FINE" message AND the "no new src/ files" message. So they enforce the same behavior.
+
+**Recommendation:** Consolidate. Pick one as canonical (suggest `file_naming_convention` — the original source location) and either:
+- (a) delete `large_files_are_fine` and redirect its v1.md to point at `file_naming_convention`, OR
+- (b) keep both but rewrite `large_files_are_fine`'s v1.md to focus on a distinct aspect (e.g., the "10K+ line files are fine" rationale in production codebases) so the bodies are no longer identical.
+
+**Source of duplication:** `file_naming_convention` lifted `AGENTS.md:62-69 + workflow.md:45`; `large_files_are_fine` lifted `AGENTS.md:62-67`. Both harvest passes pulled the same 5–6 bullet block from the same source.
+
+---
+
+## Near-duplicates (keep both, document distinction)
+
+These pairs/clusters share substantial overlap but each directive adds a specific angle the other does not. The user has explicitly chosen the keep-both approach for the first three.
+
+### 1. `tdd_red_green_required` ↔ `test_must_fail_for_believed_reason`
+
+**Both about:** the TDD red-green order.
+
+**Distinction:**
+- `tdd_red_green_required` (workflow.md:78-100): the **iron law** — write the failing test FIRST, then see it fail, then implement.
+- `test_must_fail_for_believed_reason` (test-driven-development SKILL.md:113-128): the **verification step** — when the test fails, the failure must be for the *reason you believe* (not a typo, not an import error). The test must actually exercise the missing behavior.
+
+**Why both:** the first is a sequence; the second is a quality check on the output of step 1. A test that fails for the wrong reason is worse than no test at all.
+
+### 2. `verify_before_editing` ↔ `evidence_before_completion_claims`
+
+**Both about:** verification before claiming something is done/correct.
+
+**Distinction:**
+- `verify_before_editing` (edit_workflow.md:14-24, Rule 2): **pre-edit** verification — `py_check_syntax`, `get_file_slice`, check the contract before touching the file.
+- `evidence_before_completion_claims` (verification-before-completion SKILL.md:17-37): **post-implementation** verification — run the test, observe the output, then claim the test passes. No "should work" or "looks good".
+
+**Why both:** different lifecycle stages. Pre-edit verification is about not making mistakes; post-implementation verification is about not lying about whether the work is done.
+
+### 3. `decompose_or_isolate_never_offload` ↔ `never_inherit_session_history_to_subagent`
+
+**Both about:** sub-agent dispatch discipline.
+
+**Distinction:**
+- `decompose_or_isolate_never_offload` (nagent_takeaways_v3:71-72): **when to delegate** — only when the task is decomposed (genuinely complex) or context-isolated (sub-agent needs different context). Never offload a single-action.
+- `never_inherit_session_history_to_subagent` (dispatching-parallel-agents:9-10 + subagent-driven-development:9-14): **how to give context** — explicit, surgical, no inherited session history.
+
+**Why both:** the first gates delegation (should you do it?); the second governs the contract once you've decided to delegate. They are sequential, not redundant.
+
+### 4. `tier1_first_commit_6file_acknowledgment` ↔ `acknowledgment_in_first_commit`
+
+**Both about:** the "first commit must include 'TIER-N READ <list> before <task>'" rule.
+
+**Distinction:**
+- `tier1_first_commit_6file_acknowledgment`: applies to **Tier 1** (Orchestrator). 6 files in the read list. The Tier 1 directive even has a "See also" link to the Tier 2 rule.
+- `acknowledgment_in_first_commit`: applies to **Tier 2** (Tech Lead autonomous). 11 files in the read list. Different file list because Tier 2 needs more context (the pre-action-required-reading list expanded for autonomous execution).
+
+**Why both:** tier-specific contracts. Same shape, different scope. Removing one would break the tier-isolation guarantee.
+
+### 5. `git_hard_bans` ↔ `timeline_is_immutable`
+
+**Both about:** never rewriting git history.
+
+**Distinction:**
+- `git_hard_bans` (AGENTS.md:59-60 + workflow.md:417-430): **operational rule** — `git restore`, `git checkout -- <file>`, `git reset`, `git stash*` are BANNED; ASK FIRST.
+- `timeline_is_immutable` (tier2-autonomous:92-115): **philosophical rule** — when you make a wrong commit, write a FORWARD commit that fixes it. The git timeline is immutable. Banned: `git revert`, `git reset --hard`, `git stash*`.
+
+**Why both:** `git_hard_bans` is the pre-command check (operational, "don't run this command"). `timeline_is_immutable` is the post-failure response (philosophical, "fix forward, not back"). Different angles, both essential. Overlap on `git reset` and `git stash*` is a feature (reinforcement), not a bug.
+
+### 6. `file_naming_convention` (after consolidation) ↔ `no_new_src_files_without_permission`
+
+**Status:** Even after consolidating #1 above, the `file_naming_convention` directive still overlaps with `no_new_src_files_without_permission`. They both say "no new src/ files without user authorization" but with different framings.
+
+**Distinction:**
+- `file_naming_convention` (AGENTS.md:62-69 + workflow.md:45): the rule is embedded in a larger "File Size and Naming Convention" block that ALSO covers the "large files are FINE" message and the "scripts/audit_*.py, tests/test_*.py, docs/*.md are the only new files" rule.
+- `no_new_src_files_without_permission` (AGENTS.md:71-78): a focused, narrow rule on the hard limit of new top-level `src/<thing>.py` files. Source is a different section of AGENTS.md (the "Hard rule" section).
+
+**Why both:** `file_naming_convention` is the holistic file-policy view; `no_new_src_files_without_permission` is the focused enforcement statement. Same source family (AGENTS.md), different sections, different specificity.
+
+**Note:** This cluster is best resolved by the consolidation of `file_naming_convention` + `large_files_are_fine` (item 1), then keeping `no_new_src_files_without_permission` as the narrow focused rule that complements the broader `file_naming_convention`.
+
+### 7. `intelligent_signal_endpoint_pattern` ↔ `live_gui_poll_not_sleep`
+
+**Both about:** polling for asynchronous state in tests.
+
+**Distinction:**
+- `intelligent_signal_endpoint_pattern` (TODO_test_full_live_workflow §1, §4): **WHAT to poll** — a purpose-built signal endpoint, NOT derived state. The polling target is the rule.
+- `live_gui_poll_not_sleep` (workflow.md:464-475): **HOW to wait** — replace `time.sleep(N)` with a poll loop on `get_value` or `wait_for_event`. The polling mechanism is the rule.
+
+**Why both:** orthogonal rules. The first is "poll the right thing"; the second is "poll, don't sleep". Both can be violated independently (you could poll derived state, or you could `time.sleep`).
+
+---
+
+## Adjacent rules (no action, just observed)
+
+These clusters cover the same domain but enforce different sub-behaviors. They are all kept as-is.
+
+### 1. Python type ban cluster (9 directives, all from `python.md §17`)
+
+`ban_any_type`, `ban_dict_any`, `ban_dict_get_on_known_fields`, `ban_getattr_dispatch`, `ban_hasattr_dispatch`, `ban_local_imports`, `ban_optional_returns`, `ban_prefix_aliasing`, `ban_repeated_from_dict`.
+
+Each bans a SPECIFIC Python pattern. They are not duplicates — they each prohibit a different idiom. The cluster is intentionally split because each rule has a distinct violation pattern and a distinct fix.
+
+### 2. Error-handling cluster (4 directives)
+
+`result_error_pattern` (Result[T] for failures), `nil_sentinel_pattern` (nil-sentinel for None), `ban_optional_returns` (no Optional[T] returns), `boundary_layer_exception` (the one allowed exception at the wire).
+
+These are complementary parts of the same doctrine. `ban_optional_returns` is the prohibition; `result_error_pattern` and `nil_sentinel_pattern` are the two valid alternatives; `boundary_layer_exception` is the carve-out. All four are required.
+
+### 3. Verification cluster (5 directives)
+
+`verify_before_editing` (pre-edit), `verify_critique_before_implementing` (review feedback), `verify_clean_baseline_before_starting` (start of session), `verify_tests_before_offering_completion_options` (end of implementation), `evidence_before_completion_claims` (any completion claim).
+
+Five different stages of the verification lifecycle. Each is named after the stage it applies to. Not duplicates — each would apply independently.
+
+### 4. Test discipline cluster (10+ directives)
+
+`tdd_red_green_required`, `test_must_fail_for_believed_reason`, `test_passing_immediately_proves_nothing`, `delete_means_delete_no_reference`, `test_narrow_not_kitchen_sink`, `test_instantiation_not_mock_away`, `ban_arbitrary_core_mocking`, `no_skip_markers_as_avoidance`, `opt_in_integration_test_via_env_var_marker`, `test_classification_via_import_presence`, `three_tier_test_strategy_for_fragile_subsystems`, `test_sandbox`, `no_real_io_during_tests`, `enforce_no_real_toml_in_tests`.
+
+Each is a distinct rule about a different test anti-pattern or discipline. The cluster is intentionally split. The TDD subset (first 4) reinforces each other but each addresses a different failure mode.
+
+### 5. Subagent/MMA cluster (8 directives)
+
+`tier1_orchestrator_no_implementation`, `tier3_worker_amnesia`, `tier4_qa_compressed_fix`, `token_firewall_prevents_bloat`, `decompose_or_isolate_never_offload`, `never_inherit_session_history_to_subagent`, `subagent_returns_artifact_not_transcript`, `agent_prompt_one_independent_domain`.
+
+The first 4 are tier-specific contracts (what each tier sees/does). The last 4 are dispatch-quality rules (when to delegate, how to give context, what to return, how to scope prompts). All distinct.
+
+### 6. ImGui rendering cluster (8 directives)
+
+`imgui_scope_entered_flag_for_no_op_return`, `imgui_scope_verification`, `imscope_tuple_return_per_scope_override`, `modal_explicit_opened_list_for_lifecycle`, `view_composes_does_not_leak_into_theme_get_color`, `float_only_math_for_visual_transforms`, `missing_data_renders_as_em_dash_not_crash`, `interceptor_activates_only_on_matching_shape`.
+
+All about ImGui rendering specifics. Each addresses a different rendering pitfall.
+
+### 7. Git/discipline cluster (6 directives)
+
+`git_hard_bans`, `timeline_is_immutable`, `atomic_per_task_commits`, `verbose_commit_message_ban`, `no_skip_markers_as_avoidance`, `master_branch_default`.
+
+`git_hard_bans` and `timeline_is_immutable` are near-duplicate (see above); the rest are distinct. `master_branch_default` is a sandbox-specific default-branch rule.
+
+### 8. RAG/vector cluster (3 directives)
+
+`rag_six_rules`, `chroma_cache_path`, `graceful_optional_dependency_degradation`.
+
+`rag_six_rules` is the meta-rule; `chroma_cache_path` is a specific path rule; `graceful_optional_dependency_degradation` is about optional deps (not just RAG, but applies). All distinct.
+
+### 9. Spec/Plan cluster (8 directives)
+
+`spec_template_required_6_sections`, `spec_self_review_four_checks`, `spec_review_before_quality_review`, `plan_steps_2_to_5_minutes_each`, `plans_no_placeholders_or_tbds`, `review_plan_critically_before_executing`, `brainstorm_even_for_simple_projects`, `design_leads_with_recommendation`.
+
+All about the spec/plan workflow. Each is a specific step in the workflow (template, self-review, quality review, plan granularity, no placeholders, critical review, brainstorm before implementation, lead with recommendation).
+
+### 10. Tier 2 sandbox cluster (8 directives)
+
+`use_batched_test_runner`, `prefer_targeted_tier_runs`, `no_output_filtering`, `ban_appdata_paths`, `throwaway_scripts_isolated_subdir`, `per_conversation_scratch_dir`, `master_branch_default`, `tier2_post_track_ruff_mypy_audit`, `tier2_pre_flight_audit_gates`, `tier2_pre_commit_deletion_and_diff_check`, `end_of_track_report_required`, `per_phase_metric_regression_fix`, `timeline_is_immutable`.
+
+All Tier 2-specific operational rules. Each governs a different aspect (test runner, test scope, output, paths, scripts, scratch dirs, branch, audits, pre-commit, post-track, end-of-track, regression metric, git history).
+
+### 11. State/lifecycle cluster (5 directives)
+
+`controller_property_delegation_no_dual_state`, `config_state_owner`, `state_visible_at_the_right_layer`, `undo_redo_100_snapshot_capacity`, `reset_session_preserves_project_path`.
+
+All about state. `controller_property_delegation_no_dual_state` and `config_state_owner` are about WHO owns state (Controller). `state_visible_at_the_right_layer` is about WHERE state lives (files). `undo_redo_100_snapshot_capacity` is the HistoryManager contract. `reset_session_preserves_project_path` is a specific reset_session behavior.
+
+### 12. DSL/Intent cluster (5 directives)
+
+`dsl_uses_first_class_spans_for_errors`, `intent_signal_postfix_not_xml`, `pipeline_immediate_mode_no_object`, `no_conductor_yaml_for_artifacts`, `no_content_duplication_across_agent_docs`.
+
+All about the DSL and artifact format. Each is a different aspect (spans, signal format, pipeline mode, no YAML, no content duplication).
+
+### 13. Debugging methodology cluster (4 directives)
+
+`reproduction_before_fix`, `single_hypothesis_minimal_test`, `deduction_loop_limit`, `three_fix_failures_question_architecture`.
+
+All phases of the systematic-debugging methodology. Phase 1: reproduce. Phase 3: single hypothesis. Loop limit. Phase 4.5: after 3 fails, question architecture. All distinct.
+
+### 14. Code review cluster (5 directives)
+
+`no_performative_agreement_in_review`, `verify_critique_before_implementing`, `clarify_unclear_review_before_partial_impl`, `review_after_each_task_not_end`, `spec_review_before_quality_review`.
+
+All about receiving/reviewing feedback. Different aspects (don't be performative, verify the critique, clarify unclear items, review per task not at end, spec review before quality review).
+
+---
+
+## Verified-distinct (high-confidence)
+
+These pairs I explicitly considered and confirmed are NOT duplicates. Listed so the user can see the negative space.
+
+1. `verify_critique_before_implementing` vs `verify_before_editing` — both start with "verify" but the first is about code-review feedback (from a reviewer), the second is about the file state (pre-edit). Different inputs.
+2. `reproduction_before_fix` vs `verify_clean_baseline_before_starting` — both "verify first" but the first is per-bug (Phase 1 of debugging), the second is per-session (start of worktree).
+3. `per_conversation_scratch_dir` vs `ban_appdata_paths` — both about paths but the first is about per-instance isolation (avoid collision), the second is about NEVER using OS temp dirs.
+4. `reproduction_before_fix` vs `single_hypothesis_minimal_test` — both about debugging but the first is Phase 1 (reproduce), the second is Phase 3 (hypothesis testing).
+5. `evidence_before_completion_claims` vs `verify_tests_before_offering_completion_options` — both verification before completion but the first is a general meta-rule (any claim), the second is a specific gate (verify tests before offering the 4 options).
+6. `tier2_pre_flight_audit_gates` vs `tier2_pre_commit_deletion_and_diff_check` — both Tier 2 "pre-X" checks but the first is pre-SESSION (start), the second is pre-COMMIT (before each commit).
+7. `intelligent_signal_endpoint_pattern` vs `live_gui_poll_not_sleep` — see near-duplicate #7 above; distinct but adjacent.
+8. `ban_arbitrary_core_mocking` vs `test_instantiation_not_mock_away` — both anti-mock but the first is about the `unittest.mock.patch` tool, the second is about exercising real instantiation paths.
+9. `comprehensive_logging` vs `state_visible_at_the_right_layer` — both about state but the first is about LOGGING (action audit trail), the second is about STATE FILES (user-inspectable state).
+10. `audit_before_claiming_current_state` vs `inherited_cruft_ask_first` — both about "be careful with prior-session state" but the first is about REPORTS (don't claim current state from old reports), the second is about FILES (when file is broken from previous agent).
+11. `controller_property_delegation_no_dual_state` vs `config_state_owner` — both about Controller-as-source-of-truth but the first is about App property delegation (don't duplicate state), the second is about config I/O ownership (only Controller writes to disk).
+12. `inherited_cruft_ask_first` vs `verify_clean_baseline_before_starting` — both "before starting" rules but the first is about a broken file (ask the user), the second is about pre-existing test failures (flag them).
+13. `use_batched_test_runner` vs `run_full_tier_after_phase_refactor` vs `prefer_targeted_tier_runs` — all about test running but the first is the runner (batched, not direct pytest), the second is the post-refactor scope (full tier), the third is the per-task scope (targeted).
+14. `no_skip_markers_as_avoidance` vs `opt_in_integration_test_via_env_var_marker` — both about test markers but the first is a meta-rule (skip is documentation, not avoidance), the second is a specific opt-in pattern (env-var-gated marker, not skip).
+
+---
+
+## Recommendation summary
+
+**Action recommended:** consolidate `file_naming_convention` ↔ `large_files_are_fine` (item 1 in True duplicates). The bodies are literally identical; one of the two should be deleted or its body rewritten to focus on a distinct aspect.
+
+**Action NOT recommended:** all 6 near-duplicate pairs above. Each pair has a meaningful distinction that justifies keeping both. The user's pre-flagged pairs (`tdd_red_green_required` ↔ `test_must_fail_for_believed_reason`; `verify_before_editing` ↔ `evidence_before_completion_claims`; `decompose_or_isolate_never_offload` ↔ `never_inherit_session_history_to_subagent`) confirm the keep-both posture for this library.
+
+**Observation:** The 14 adjacent-rule clusters are all intentional. Each cluster covers a domain (Python type bans, ImGui rendering, subagent dispatch, etc.) but each directive in the cluster enforces a distinct sub-behavior. Splitting the domain into multiple rules is the correct granularity for directive libraries — one rule per anti-pattern, one rule per stage, one rule per tier.