chore: remove dead code and consolidate duplicated enforcement logic - #53
Merged
Conversation
Prompted by a codebase sweep for dead code and needlessly complex patterns: - Remove unused LogCategory.auth/.report cases and name the magic 11/10 literals in LogFilename.parse. - RulePolicy's hard-lock predicates now delegate to UninstallProtectionPolicy instead of reimplementing the same logic. - Rename RuleActivation's blockEnd -> currentBlockEnd and extract UsageReportFormatter's sort into a named heaviestUsageFirst comparator, both replacing an explanatory comment with a self-documenting name. - Add Shared/Platform/DeviceActivityFactory with nonRepeatingSchedule/thresholdEvents, replacing 7 duplicated DeviceActivityEvent/DeviceActivitySchedule construction blocks across RuleScheduler, DeviceActivityMonitorExtension, and ShieldActionExtension. Also corrects two comments describing the now-superseded N=2 foreground-arming design. - Add RuleSnapshotDTO.isEligible(kind:at:calendar:) to replace a duplicated 4-part guard across 3 LimitEnforcement handlers, and a UUID.logTag extension to replace the repeated .uuidString.prefix(8)/`rid` idiom across 8 files. All three stages verified independently and combined against the full 397-test suite (0 failures throughout); code-reviewer and security-reviewer both passed with no critical/high/medium findings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comments should be a relative rarity; a rename is the first resort before reaching for a comment, and naming must be unambiguous enough for an agent reading only identifiers to infer behavior. Explicitly exempts the feature-spec /// doc comments indexed by AGENTS.md's "Rules feature map" — those are the behavior spec, not restatements, and remain required. The same rule was also added to the user's global ~/.claude/rules/common/coding-style.md (outside this repo). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.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
1 participant
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
A dead-code / complexity / naming cleanup requested after an audit of the codebase, executed via
spec-driven-developmentin 3 verified stages plus a post-review polish fix.Dead code removed:
LogCategory.authandLogCategory.reportenum cases (Shared/Diagnostics/LogEntry.swift) — confirmed zero non-declaration references repo-wide.Duplication consolidated:
RulePolicy's hard-lock predicates now delegate toUninstallProtectionPolicyinstead of reimplementing identical logic.Shared/Platform/DeviceActivityFactory.swiftreplaces 7 duplicatedDeviceActivityEvent/DeviceActivityScheduleconstruction blocks acrossRuleScheduler,DeviceActivityMonitorExtension, andShieldActionExtension.RuleSnapshotDTO.isEligible(kind:at:calendar:)replaces a duplicated 4-part eligibility guard across 3LimitEnforcementhandlers.Naming over comments (per a newly added project rule — see below):
LogFilename.parse's magic11/10literals replaced with named constants.RuleActivation'sblockEndrenamed tocurrentBlockEnd, removing the comment it needed.UsageReportFormatter's inline sort closure extracted into a namedAppUsageRow.heaviestUsageFirstcomparator.UUID.logTagextension replaces the repeated.uuidString.prefix(8)idiom and crypticridlocal (renamedruleTag) across 8 files.DeviceActivityMonitorExtension.swiftdescribing an obsolete "N=2" design corrected to describe the current N=1 design.Project rule added (
Docs/AGENT_SWIFT_GUIDELINES.md, separate commit): comments should be a relative rarity — prefer a descriptive rename first — with an explicit carve-out for the feature-spec///doc comments, which remain required.Verification
code-reviewerandsecurity-reviewer(opus) both reviewed the full diff in parallel: 0 critical/high/medium findings from either. Code review flagged one LOW polish note (an implicit vs. explicitcalendar:argument inShieldActionExtension), which has been applied.origin/main(clean, no conflicts) and re-verified: 402/402 passing (397 + 5 new tests from main).RuleActivation/RuleStatusderivation path. Did not drive taps through Rules/Settings interactively (no idb/Maestro-permission/computer-use path was available in-session for this run); this refactor touches zero View files, and the full XCUITest suite (which does drive those screens) passed before and after.Test plan
RulePolicy→UninstallProtectionPolicydelegation and theLimitEnforcementguard consolidation, since both touch Hard Mode / enforcement-eligibility logic (reviewed and verified equivalent by both automated reviews, but worth a human's own look given what they gate)🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com