From 9ffa469f489d6394e4be9907dd3ee36b371584e4 Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sat, 4 Jul 2026 13:12:23 -0400 Subject: [PATCH] docs(review): semantic dedup review of 172 directives --- .../DIRECTIVE_DEDUP_REVIEW_20260703.md | 230 ++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 docs/reviews/DIRECTIVE_DEDUP_REVIEW_20260703.md diff --git a/docs/reviews/DIRECTIVE_DEDUP_REVIEW_20260703.md b/docs/reviews/DIRECTIVE_DEDUP_REVIEW_20260703.md new file mode 100644 index 00000000..4acfd9d7 --- /dev/null +++ b/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/.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 before '" 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 -- `, `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/.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.