fix: wire advocacy phases into every language config#40
Merged
Conversation
`desloppify scan` was silently skipping advocacy_language, advocacy_security, and advocacy_tool_presence detectors on 9 of 11 languages — only javascript and go had manual wiring. Confirmed via e2e: scan a Python file containing "kill two birds with one stone" + "whitelist" → status shows the Advocacy language dimension exists with "10 checks" but 0 findings; direct call to detect_advocacy_language() on the same file finds 4 instances. Root cause: each LangConfig.phases list is built manually. javascript appended advocacy phases via `cfg.phases.append(...)` after generic_lang returned; go inlined them in its phases list. python, typescript, csharp, cxx, dart, gdscript, rust, and every generic-only language never had them. Fix: move advocacy wiring into the shared tail (shared_subjective_duplicates_tail) that every language config already calls. One canonical location, one shared helper (shared_advocacy_phases), no per-language churn when phases evolve. Removed the now-duplicate manual wiring in javascript and go. Also: added the 3 advocacy phase labels to SHARED_PHASE_LABELS in capabilities.py so capability_report stops bucketing them under "linting" in the langs command output. Verified post-fix: scan on the same Python test file now prints `[15/20] Advocacy language... 4 instances → 4 issues` and `show advocacy_language` returns the persisted findings with full suggestion/alternative data. All existing tests pass (after updating test_phase_builders.py assertions for the new tail shape). Regression test added: test_advocacy_phase_coverage.py asserts every dedicated LangConfig AND generic_lang include all 3 advocacy phases. Would have caught the original bug.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR consolidates advocacy detector phase wiring by introducing a shared ChangesAdvocacy Phase Consolidation Campaign
Sequence DiagramNot applicable—this is a refactoring consolidation with no new feature control flow or multi-component interactions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 tasks
samtuckerdavis
added a commit
that referenced
this pull request
May 13, 2026
After PR #40, running the full suite on Windows surfaced 34 failures that weren't visible in CI (Linux). All were Windows-specific quirks falling into five categories. Each was root-caused and fixed without changing Linux behavior. Full suite now green on Windows Python 3.12: Before: 34 failed, 6796 passed After: 0 failed, 6830 passed, 6 skipped ## Path-separator normalization (production code, 7 files) Several modules built prefix strings with `os.sep` ("\\" on Windows) and then compared against POSIX-style paths emitted by other tools or test fixtures. The mismatched separators caused silent miss-matches. Each fix normalizes both sides to "/" for comparison while preserving native separators in the output where production callers expect them. - `engine/detectors/coupling.py::_rel_to_root` — also strip drive letter when root resolved to `C:/...` but the caller still passes POSIX-rooted prefix strings (e.g. tests using `/project/...`). - `engine/detectors/coverage/mapping_analysis.py::_build_prod_by_module` — normalize prod_file before prefix-strip; on Windows the mixed-slash inputs were missing the project-root prefix and producing module names like `.repo.pkg.util` instead of `pkg.util`. - `engine/detectors/test_coverage/discovery.py::_normalize_graph_paths, _discover_scorable_and_tests` — same pattern; also kept native- separator output for callers that round-trip native paths (test_normalize_graph_paths_converts_absolute_to_relative). - `engine/policy/zones.py::_match_pattern` — normalize both pattern and rel_path to forward slashes. Without this, an absolute Windows path pattern (`C:\...\test_creds.py`) hit the "exact basename" branch and returned False, causing the security detector to scan files in TEST zones it should have skipped. - `languages/_framework/treesitter/imports/resolvers_backend.py` — `os.path.normpath` around every returned candidate (go/rust/java/ kotlin/cxx/csharp/dart/scala/swift). Forward-slashed src-root strings like "src/main/java" combined with native scan_path produced mixed separators ("C:\...\src/main/java\com\acme\App.java") that didn't match the platform-native expected paths. - `languages/_framework/treesitter/imports/resolvers_scripts.py:: resolve_js_import` — normpath each candidate before isfile check. The "/index.js" suffix on a Windows path produced mixed separators. - `languages/javascript/test_coverage.py::resolve_import_spec` — normalize candidate after `os.path.normpath` for set-membership check against POSIX-keyed production files. ## Test path/separator fixes (3 tests) - `test_state_path_from_explicit_state_arg` — `Path.as_posix()` for cross-platform string equality. - `test_external_adapters.py::TestCollectExcludeDirs::test_includes_*, test_deduplicates` — use `os.path.basename` instead of rsplit("/") which split on the wrong separator on Windows. - `test_writes_to_file` (holistic_review remediation) — explicit `encoding="utf-8"` on the read; locale default (cp1252 on Windows) mangled the middle-dot unicode the writer emits as UTF-8. ## Skip Unix-only test path - `test_timeout_kills_jscpd_process_group` — `pytest.mark.skipif` on `hasattr(os, 'getpgid')`. The kill-by-process-group path is POSIX only; on Windows the timeout handling uses TerminateProcess, which has its own test. ## chmod assertion gated by platform - `test_orchestrator_pipeline_writes_exact_cli_helper` — skip the `st_mode & 0o111` exec-bit assert on Windows. Windows uses ACLs, not POSIX mode bits; the helper is invoked via bash/PYTHONPATH and doesn't need the exec bit set on Windows. ## cmd /c wrapping (5 tests, 1 cross-cutting fixture) - On Windows when an executable (acli, codex) isn't on PATH, `_resolve_executable` returns `["cmd", "/c", name]`, then `_wrap_cmd_c` collapses everything after `/c` into a single string for cmd.exe's tokenizer. Correct production behavior — but it obscures the per-arg list shape several tests assert on. - For 3 rovodev tests + 1 codex_batch test: per-test monkeypatch of `_resolve_executable` to return `[name]` directly. - For 12 `TestCmdReviewPrepare::test_do_run_batches_*` tests: added a class-level autouse fixture that does the same. Plus added `**kwargs` to all 7 `fake_subprocess_run` signatures + `fake_run` in `test_run_codex_batch_writes_live_status_before_completion`. The runner adds `input=stdin_text` to `subprocess.run` kwargs whenever `_prompt_via_stdin(prompt)` is true — which is always on Windows (`return sys.platform == "win32" or len(prompt) > _PROMPT_ARG_MAX_CHARS`). Without **kwargs, the fakes raised TypeError, hit `_run_via_subprocess`'s except branch, and returned exit_code=1. None of these touch Linux behavior. CI on Linux 3.11 was already green; this brings Windows 3.12 to parity. Co-authored-by: Original Gary <276612211+OpenGaryBot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
desloppify scanwas silently skipping all 3 advocacy detectors (advocacy_language, advocacy_security, advocacy_tool_presence) on 9 of 11 languages. Only javascript and go had the wiring. python, typescript, csharp, cxx, dart, gdscript, rust, and every generic-only language never ran advocacy phases — defeating the fork's core differentiator.How it surfaced
E2E test on the merged sync (#39): scan a Python file with
"kill two birds with one stone"+"whitelist"→desloppify statusshowed the Advocacy language dimension exists with "10 checks" (so the detector was registered in the catalog)desloppify show advocacy_languagereturned 0 issuesdetect_advocacy_language()on the same file found 4 instances with full suggestion/alternative dataSo detector code was fine — the bug was upstream of detection: the phase wasn't being scheduled.
Root cause
Each
LangConfig.phaseslist is built manually per language. The two languages that had advocacy wiring did it inconsistently:generic_lang()returned (cfg.phases.append(detector_phase_advocacy_language()))Every other language config (and every generic_lang-built plugin) was just missing the phase entries. Nobody had a single source of truth for "all OP languages should run advocacy."
Fix
Move advocacy wiring into
shared_subjective_duplicates_tail()— the helper that everyLangConfig.phaseslist (and thegeneric_langbuilder) already calls. One canonical location.New helper
shared_advocacy_phases()returns the 3 phases; the tail prepends them beforeSubjective review. Removed the now-duplicate manual wiring in javascript and go.Also added the 3 advocacy labels to
SHARED_PHASE_LABELSincapabilities.pysocapability_reportstops bucketing them under"linting"in thedesloppify langsoutput.Verification
E2E rerun post-fix on the same Python test file:
And
desloppify show advocacy_languagenow lists:T2 [high] "kill two birds with one stone" → use "accomplish two things at once" instead (idioms)T2 [high] "whitelist/blacklist" → use "allowlist/denylist" instead (terminology)Regression coverage
New
test_advocacy_phase_coverage.pyparametrizes over all 8 dedicatedLangConfigs + asserts ongeneric_lang. Each must contain all 3 advocacy phase labels. Would have caught the original bug — adding it forces future language-config additions to go throughshared_subjective_duplicates_tail()(or otherwise prove they include advocacy).Updated
test_phase_builders.pyfor the new tail shape (length 3 → 6, with advocacy phases at positions 0–2).Test plan
Summary by CodeRabbit
New Features
Tests