chore: delete orphaned mechanisms and enforce the rule in CI - #603
Merged
Merged
Conversation
#601 was not really about sudoers. lib/systems-capabilities.sh had a correct, validated grant installer whose only consumer was retired; the function stayed with zero call sites. The next component that needed one did not find it and wrote a weaker copy that installed before validating. The duplicate was the symptom; the cause is that an orphaned mechanism is invisible -- it reads as supported infrastructure, it has a docblock, and nothing tells the next author it is already dead. An audit found ten across the repo, 120 lines. Most are vestigial entrypoints left by the convergence refactor: runtime_guidance_desired_state_apply, integration_adapters_verify and friends are one-line pass-throughs to reconciler_apply_plan and reconciler_verify_plan, superseded by convergence_apply/convergence_verify and called by nothing. install_chat_bridge still documented itself as 'the setup-time entrypoint, dispatched from setup.sh' -- setup.sh has gone through the convergence orchestrator for some time. Deleting _resolve_claude_code_agent_slug orphaned its only helper, which the audit then caught on the next pass: dead code hides more dead code. tests/dead-mechanism.sh makes the rule enforceable. Bash dispatches indirectly, so it reads the three dispatch prefixes out of the source rather than hardcoding them, and treats anything under a discovered prefix as reachable -- a deliberate blind spot, because a checker with false positives gets an allowlist, then gets ignored, then gets deleted. Judgement does not catch this: two implementations of sudoers installation survived months of review. A grep does, on every push. Refs #601
chubes4
added a commit
that referenced
this pull request
Sep 17, 2026
…604) The sibling finding to #603. That audit asked which mechanisms have no consumer; this one asks which have two bodies. Escaping was written three times. Two JSON escapers handled backslash, quote and newline; lib/agent-state-ownership.sh's handled only backslash and quote, so a value containing a newline emitted a literal newline inside a JSON string literal -- invalid JSON, in the one copy whose output is machine-read. Nothing exercised the newline case, which is why the drift survived. XML text escaping was triplicated across the kimaki bridge and both launchd services, still identical, waiting to do the same thing. json_escape and xml_escape now live in lib/common.sh with one implementation each. Escaping is exactly the code that must exist once: short enough to feel harmless to re-type, consequential enough that a divergence is a correctness bug rather than a style difference. setup_ai_gateway and upgrade_ai_gateway had identical bodies. Two names for one behaviour reads as 'these differ somehow' to anyone who has not diffed them. One ai_gateway_apply, called from both. tests/duplicate-mechanism.sh makes the rule enforceable, comparing normalised bodies and reporting only exact matches -- near-duplicates are a judgement call, and a checker that makes judgement calls gets argued with rather than fixed. tests/escaping-helpers.sh covers the behaviour, including the newline case that was missing. Verified to fail against the pre-consolidation escaper and pass against the shared one. Refs #601
chubes4
added a commit
that referenced
this pull request
Sep 17, 2026
…605) Block-level duplication, which the function-level audits in #603 and #604 could not see: the plist document frame was copy-pasted into six renderers across three bridges and two services. The copies had already disagreed. Both services ran their values through xml_escape; all three bridges interpolated them raw. A SITE_PATH, service home, log directory, model name, or bot token containing an ampersand rendered a plist that is not well-formed XML, which launchd refuses to load -- so the agent never starts, with no failing test anywhere to say why. Verified against main: a path with '&' in it produces a ParseError at the WorkingDirectory line. The snapshot fixtures could not catch this because they use tidy values like /var/www/site. A golden file locks in whatever was rendered the day it was written, including a bug. Only the frame is shared. The bodies genuinely differ -- the worker schedules with StartInterval where the bridges use KeepAlive, and the WordPress service renders no EnvironmentVariables -- so they stay as readable heredocs. A single renderer taking seven parameters and three optional blocks to absorb that variation would be harder to read than the duplication it removed. All 8 committed snapshots remain byte-identical, so this changes nothing for values that were already safe. tests/plist-rendering.sh renders every plist with a value carrying & < > and asserts the result parses as XML and still contains the value. It fails against main and passes here.
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
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.
Implements the rule proposed in #601 §6, and applies it.
Why this is the follow-up to #601
#601 was not really a bug about sudoers.
lib/systems-capabilities.shhad a correct, validated grant installer; its only consumer was retired; the function stayed with zero call sites. The next component that needed a grant didn't find it and wrote its own — which installed the file before validating it, and could leave a rejected policy in/etc/sudoers.dthat locks every user out ofsudo.The duplicate was the symptom. The cause is that an orphaned mechanism is invisible: it reads as supported infrastructure, it has a docblock describing what it does, and nothing tells the next author it's already dead.
Judgement doesn't catch this — two implementations of sudoers installation survived months of review. A grep does, cheaply, on every push.
What the audit found
Ten functions, 120 lines. Most are one story: vestigial entrypoints left behind by the convergence refactor.
Once
convergence_apply/convergence_verifybecame the single entrypoint, each module's own apply/verify pass-through stopped being called and stayed — three alternative front doors to one room, all bricked up.install_chat_bridgeis the sharpest example. Its docblock:setup.shhas gone through the convergence orchestrator for some time and never calls it. The comment documents an architecture that no longer exists, which is worse than no comment: it's a confident wrong answer to "how does the bridge get installed?"The rest:
reconciler_mark_changed(a dead twin ofreconciler_adapter_changed, which has 14 call sites — the same duplicate-mechanism shape as #601, caught before it could be the survivor),runtime_sync_instructionsinruntimes/codex.sh(a hook the contract dropped in favour ofruntime_generate_instructions, which all three runtimes implement),homeboy_json_array,_datamachine_worker_shell_quote, and_resolve_claude_code_agent_slug.Dead code hides more dead code. Removing
_resolve_claude_code_agent_slugorphaned_dm_agent_slug_exists, its only consumer — caught on the audit's next pass. Run to a fixpoint, the repo is clean.On
_resolve_claude_code_agent_slugspecifically: it's unreachable, soAGENT_SLUGalready stays empty on upgrade unless--agent-slugis passed. That is a handled state —hooks/dm-agent-sync.shfalls back to discovering all active agents, as the function's own comment said it would. Behaviour is unchanged by deleting it. If pinning a single agent on upgrade is wanted, it should be wired to a caller, not left as a 37-line unreachable statement of intent.The check
tests/dead-mechanism.sh, wired into CI. Bash dispatches indirectly, so a function can be live without its name ever appearing at a call site. Rather than hardcode the dispatchers, it reads the prefixes out of the source:so a new dispatcher is understood the moment it's written. Anything under a discovered prefix is treated as reachable — a deliberate blind spot, documented in the file. Dead code under
bridge_orguidance_won't be caught. Narrow and honest beats broad and wrong: a checker with false positives gets an allowlist, then gets ignored, then gets deleted.ALLOWEDexists and is empty; if it grows, the rule is being worked around rather than followed.Before:
After, at fixpoint:
Verification
Full suite on Linux: 71 pass, 4 fail. The same 4 fail on a pristine
mainclone on the same host —datamachine-worker,opencode-general-dispatch,opencode-subagents,workspace-materialization— all environment-dependent there (/tmpis world-writable,wpresolves to an absolute path), not regressions. Verified by cloningmainand running them side by side rather than assuming.bash -nclean across every.sh. No dangling references to any removed name.tests/ci-coverage.sh— the sibling check that catches tests CI never runs — passes with the new job wired.AI assistance: audited and implemented by Claude Opus 4.5 via Claude Code, driven by Chris. The model wrote the reachability audit, manually verified its output against the dispatchers to confirm zero false positives before trusting it, removed the dead set to a fixpoint, and ran the full suite against both this branch and pristine main on a Linux host to separate regressions from pre-existing environmental failures.