diff --git a/Sources/mcs/Core/CLIOutput.swift b/Sources/mcs/Core/CLIOutput.swift index 06b03711..1ebb11e3 100644 --- a/Sources/mcs/Core/CLIOutput.swift +++ b/Sources/mcs/Core/CLIOutput.swift @@ -1,4 +1,26 @@ import Foundation +import os + +/// Shared, mutable tally of warnings emitted through a `CLIOutput`. +/// +/// A reference type so that value-type `CLIOutput` copies (e.g. the one handed +/// to `DestinationCollisionResolver`) all increment the same counter. Lets a +/// caller like `DoctorRunner` faithfully count every warning shown — including +/// advisories emitted outside the doctor check loop. +/// +/// `Sendable` so `CLIOutput` stays `Sendable` (it's captured in isolated +/// closures, e.g. via `ScriptRunner`); the lock supplies that guarantee. +final class WarningCounter: Sendable { + private let lock = OSAllocatedUnfairLock(initialState: 0) + + var count: Int { + lock.withLock { $0 } + } + + func increment() { + lock.withLock { $0 += 1 } + } +} /// Terminal output with ANSI color support and structured logging. struct CLIOutput { @@ -10,8 +32,11 @@ struct CLIOutput { /// manipulation, ANSI ornamentation) can render. Gate pickers on this. let isInteractiveTerminal: Bool let style: ANSIStyle + /// Optional tally that `warn(_:)` increments. `nil` for most callers; set by + /// callers (e.g. `DoctorRunner`) that need to count emitted warnings. + let warningCounter: WarningCounter? - init(colorsEnabled: Bool? = nil) { + init(colorsEnabled: Bool? = nil, warningCounter: WarningCounter? = nil) { if let explicit = colorsEnabled { self.colorsEnabled = explicit } else { @@ -20,6 +45,7 @@ struct CLIOutput { hasInteractiveStdin = isatty(STDIN_FILENO) != 0 isInteractiveTerminal = hasInteractiveStdin && isatty(STDOUT_FILENO) != 0 style = ANSIStyle(enabled: self.colorsEnabled) + self.warningCounter = warningCounter } // MARK: - ANSI Codes (delegate to `style`) @@ -131,6 +157,7 @@ struct CLIOutput { } func warn(_ message: String) { + warningCounter?.increment() write("\(yellow)[WARN]\(reset) \(message)\n") } diff --git a/Sources/mcs/Doctor/DoctorRunner.swift b/Sources/mcs/Doctor/DoctorRunner.swift index d4b53fda..72be5c2c 100644 --- a/Sources/mcs/Doctor/DoctorRunner.swift +++ b/Sources/mcs/Doctor/DoctorRunner.swift @@ -1,5 +1,12 @@ import Foundation +/// Outcome tallies from a doctor run, captured at summary time. +struct DoctorSummary { + let passed: Int + let warnings: Int + let issues: Int +} + /// Orchestrates all doctor checks grouped by section, with optional fix mode. /// /// **Scope of `--fix`**: Cleanup, migration, and trivial repairs only. @@ -15,10 +22,13 @@ struct DoctorRunner { let globalOnly: Bool let registry: TechPackRegistry - private let output = CLIOutput() + /// Counts every warning emitted through `output`, including advisories shown + /// outside the check loop (collision renames, unregistered packs, unreadable + /// state) so the summary tally is faithful to what the user saw. + private let warningCounter = WarningCounter() + private let output: CLIOutput private var passCount = 0 private var failCount = 0 - private var warnCount = 0 private var fixedCount = 0 /// Failed checks collected during diagnosis, to be fixed after confirmation. private var pendingFixes: [any DoctorCheck] = [] @@ -60,9 +70,11 @@ struct DoctorRunner { self.registry = registry self.environment = environment self.projectRootOverride = projectRootOverride + output = CLIOutput(warningCounter: warningCounter) } - mutating func run() throws { + @discardableResult + mutating func run() throws -> DoctorSummary { output.header("Managed Claude Stack — Doctor") let env = environment @@ -230,13 +242,19 @@ struct DoctorRunner { runChecks(checks) } - // Summary (before fixes, so the user sees the full picture first) + // Summary (before fixes, so the user sees the full picture first). + // Capture once: fix-phase warnings (below) must not alter the reported total. + let summary = DoctorSummary( + passed: passCount, + warnings: warningCounter.count, + issues: failCount + ) output.header("Summary") output.doctorSummary( - passed: passCount, + passed: summary.passed, fixed: 0, - warnings: warnCount, - issues: failCount + warnings: summary.warnings, + issues: summary.issues ) // Phase 2: Confirm and execute pending fixes (after summary) @@ -247,6 +265,8 @@ struct DoctorRunner { output.success("Applied \(fixedCount) fix\(fixedCount == 1 ? "" : "es").") } } + + return summary } // MARK: - Scope resolution @@ -605,7 +625,7 @@ struct DoctorRunner { } private mutating func docWarn(_ name: String, _ msg: String) { - warnCount += 1 + // warningCounter increments inside output.warn — single source of truth. output.warn("⚠ \(name): \(msg)") } diff --git a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift index 9ccaeac9..7dddbed1 100644 --- a/Tests/MCSTests/DoctorRunnerIntegrationTests.swift +++ b/Tests/MCSTests/DoctorRunnerIntegrationTests.swift @@ -357,3 +357,106 @@ struct DoctorRunnerIntegrationTests { try runner.run() } } + +// MARK: - Summary Warning-Count Tests + +/// Regression coverage for the doctor summary warning tally. The count must +/// include warnings emitted *outside* the check loop (collision renames, +/// unregistered packs), which previously printed but were never counted. +/// +/// Assertions are deltas: ambient checks (e.g. ProjectIndexCheck) may add +/// warnings of their own, so each test compares two otherwise-identical runs +/// that differ only by the single side-channel warning under test. +struct DoctorSummaryWarningCountTests { + /// The injected counter is shared across CLIOutput copies, so a warning + /// emitted through any copy (e.g. the one handed to the collision resolver) + /// is tallied. This is the mechanism the doctor summary relies on. + @Test("WarningCounter is shared across CLIOutput value copies") + func warningCounterSharedAcrossCopies() { + let counter = WarningCounter() + let output = CLIOutput(colorsEnabled: false, warningCounter: counter) + let copy = output // value-type copy, same counter instance + + #expect(counter.count == 0) + output.warn("first") + copy.warn("second") + #expect(counter.count == 2) + } + + @Test("Skill colliding with a pre-existing unmanaged file is counted in the summary") + func collisionWarningCounted() throws { + let (home, project) = try makeSandboxProject(label: "warncount-collision") + defer { try? FileManager.default.removeItem(at: home) } + + // A pack whose skill targets destination "my-skill". + let skillSource = home.appendingPathComponent("pack-my-skill") + try FileManager.default.createDirectory(at: skillSource, withIntermediateDirectories: true) + try "managed".write( + to: skillSource.appendingPathComponent("SKILL.md"), atomically: true, encoding: .utf8 + ) + let component = ComponentDefinition( + id: "test-pack.my-skill", + displayName: "my-skill", + description: "Skill", + type: .skill, + packIdentifier: "test-pack", + dependencies: [], + isRequired: true, + installAction: .copyPackFile(source: skillSource, destination: "my-skill", fileType: .skill) + ) + let pack = MockTechPack( + identifier: "test-pack", displayName: "Test Pack", components: [component] + ) + let registry = TechPackRegistry(packs: [pack]) + + // Configure the pack (no artifacts → nothing tracked at the destination). + var state = try ProjectState(projectRoot: project) + state.recordPack("test-pack") + try state.save() + + // Baseline: no pre-existing file at the destination → no collision. + var baselineRunner = makeRunner(home: home, projectRoot: project, registry: registry) + let baseline = try baselineRunner.run() + + // Now plant a pre-existing UNMANAGED skill at the same destination. + let existingSkill = project.appendingPathComponent(".claude/skills/my-skill") + try FileManager.default.createDirectory(at: existingSkill, withIntermediateDirectories: true) + try "user content".write( + to: existingSkill.appendingPathComponent("SKILL.md"), atomically: true, encoding: .utf8 + ) + + var collisionRunner = makeRunner(home: home, projectRoot: project, registry: registry) + let withCollision = try collisionRunner.run() + + // The only difference between the two runs is the collision warning. + #expect(withCollision.warnings == baseline.warnings + 1) + } + + @Test("Unregistered --pack filter warning is counted in the summary") + func unregisteredPackWarningCounted() throws { + let (home, project) = try makeSandboxProject(label: "warncount-unregistered") + defer { try? FileManager.default.removeItem(at: home) } + + let pack = MockTechPack(identifier: "test-pack", displayName: "Test Pack") + let registry = TechPackRegistry(packs: [pack]) + + var state = try ProjectState(projectRoot: project) + state.recordPack("test-pack") + try state.save() + + // Baseline: filter to the registered pack only. + var baselineRunner = makeRunner( + home: home, projectRoot: project, registry: registry, packFilter: "test-pack" + ) + let baseline = try baselineRunner.run() + + // Add an unregistered pack id to the filter — same checks, plus one + // "not registered" advisory warning. + var ghostRunner = makeRunner( + home: home, projectRoot: project, registry: registry, packFilter: "test-pack,ghost-pack" + ) + let withGhost = try ghostRunner.run() + + #expect(withGhost.warnings == baseline.warnings + 1) + } +}