From 51460efe9f4a55570e8db7279da4d80fd5b482ed Mon Sep 17 00:00:00 2001 From: onevcat Date: Sat, 29 Aug 2026 11:50:02 +0900 Subject: [PATCH] Harden the workflow parser and validator after the first review round An adversarial review of the definitions layer found seven defects; each fix landed test-first: - a duration such as `9223372036854775807h` overflowed the seconds multiplication and trapped the CLI instead of reporting `timeout_syntax` - native action inputs declared as roles (`handoff.transition` `from` and `to`) were accepted as arbitrary strings; they now must name a declared role literally - output metadata was accumulated monotonically, so a later producer without a verdict still let `{{ outputs.x.verdict }}` validate, and outputs first produced inside a loop with `until` stayed visible after a loop that may run zero times; producers are tracked per loop, the latest producer wins, and skippable loops fold their outputs - `steps: []` and typed plain scalars in string fields (`id: 1`) were accepted by the parser although the published schema rejects them - a tab in a string input default or typed text passed the single-line check although the rendered-text boundary forbids all control characters - the "no enabled profile matches suggest" warning had no data source; the app now passes the enabled Profiles' preset fields - an unreadable source directory was silently listed as empty; discovery now throws and `workflow list` answers `WORKFLOW_FAILED` Claude-Session: https://claude.ai/code/session_01YSXSCVNoycSbCmwPMvcCnw --- ProwlCLITests/WorkflowDiscoveryTests.swift | 24 ++-- .../WorkflowDocumentParserTests.swift | 33 +++++- ProwlCLITests/WorkflowFixtures.swift | 12 +- ProwlCLITests/WorkflowValidatorTests.swift | 109 ++++++++++++++++++ supacode/App/supacodeApp.swift | 10 +- .../Shared/WorkflowActionRegistry.swift | 26 ++++- .../CLIService/Shared/WorkflowDiscovery.swift | 14 +-- .../Shared/WorkflowDocumentParser.swift | 43 +++++-- .../CLIService/Shared/WorkflowValidator.swift | 102 +++++++++++++--- .../CLIService/WorkflowCommandHandler.swift | 7 +- .../WorkflowCommandHandlerTests.swift | 15 ++- 11 files changed, 342 insertions(+), 53 deletions(-) diff --git a/ProwlCLITests/WorkflowDiscoveryTests.swift b/ProwlCLITests/WorkflowDiscoveryTests.swift index 8c82ce4f..e4e8b990 100644 --- a/ProwlCLITests/WorkflowDiscoveryTests.swift +++ b/ProwlCLITests/WorkflowDiscoveryTests.swift @@ -29,8 +29,18 @@ final class WorkflowDiscoveryTests: XCTestCase { WorkflowValidationContext(scope: scope, bundledSkillIDs: ["prowl.adversarial-reviewer"]) } - func testMissingDirectoriesYieldNoFiles() { - let catalog = WorkflowDiscovery.catalog( + func testUnreadableDirectoriesThrowInsteadOfHidingTheirFiles() throws { + let user = try directory("user") + try write(WorkflowFixtures.minimal(id: "demo"), to: user, name: "demo.yaml") + try FileManager.default.setAttributes([.posixPermissions: 0o000], ofItemAtPath: user.path(percentEncoded: false)) + defer { try? FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: user.path(percentEncoded: false)) } + XCTAssertThrowsError(try WorkflowDiscovery.files(in: user, scope: .user, context: context(.user))) + XCTAssertThrowsError( + try WorkflowDiscovery.catalog(sources: WorkflowSources(bundle: nil, user: user, repo: nil), context: context)) + } + + func testMissingDirectoriesYieldNoFiles() throws { + let catalog = try WorkflowDiscovery.catalog( sources: WorkflowSources(bundle: nil, user: root.appending(path: "absent"), repo: nil), context: context) XCTAssertEqual(catalog, []) } @@ -43,7 +53,7 @@ final class WorkflowDiscoveryTests: XCTestCase { try write("ignored", to: user, name: "notes.txt") try write(WorkflowFixtures.minimal(id: "hidden"), to: user, name: ".hidden.yaml") - let files = WorkflowDiscovery.files(in: user, scope: .user, context: context(.user)) + let files = try WorkflowDiscovery.files(in: user, scope: .user, context: context(.user)) XCTAssertEqual(files.map(\.url.lastPathComponent), ["alpha.yaml", "broken.yaml", "zeta.yml"]) XCTAssertEqual(files.map(\.id), ["alpha", nil, "zeta"]) XCTAssertEqual(files.map(\.isValid), [true, false, true]) @@ -54,7 +64,7 @@ final class WorkflowDiscoveryTests: XCTestCase { func testValidationDiagnosticsFollowParseDiagnostics() throws { let user = try directory("user") try write(WorkflowFixtures.minimal(id: "prowl.mine"), to: user, name: "mine.yaml") - let files = WorkflowDiscovery.files(in: user, scope: .user, context: context(.user)) + let files = try WorkflowDiscovery.files(in: user, scope: .user, context: context(.user)) XCTAssertEqual(files.map(\.isValid), [false]) XCTAssertEqual(files[0].diagnostics.map(\.code), ["reserved_id"]) XCTAssertNotNil(files[0].definition, "A file that parses keeps its definition even when validation fails") @@ -72,7 +82,7 @@ final class WorkflowDiscoveryTests: XCTestCase { try write(WorkflowFixtures.adversarialReview, to: user, name: "override.yaml") try write(WorkflowFixtures.minimal(id: "demo"), to: repo, name: "demo.yaml") - let catalog = WorkflowDiscovery.catalog( + let catalog = try WorkflowDiscovery.catalog( sources: WorkflowSources(bundle: bundle, user: user, repo: repo), context: context) let rows = catalog.map { "\($0.file.id ?? "-") \($0.file.scope.rawValue) \($0.file.url.lastPathComponent) \($0.shadowed ? "shadowed" : ($0.file.isValid ? "wins" : "invalid"))" } XCTAssertEqual( @@ -93,7 +103,7 @@ final class WorkflowDiscoveryTests: XCTestCase { let user = try directory("user") try write(WorkflowFixtures.minimal(id: "demo"), to: user, name: "b.yaml") try write(WorkflowFixtures.minimal(id: "demo"), to: user, name: "a.yaml") - let catalog = WorkflowDiscovery.catalog( + let catalog = try WorkflowDiscovery.catalog( sources: WorkflowSources(bundle: nil, user: user, repo: nil), context: context) XCTAssertEqual(catalog.map { "\($0.file.url.lastPathComponent) \($0.shadowed)" }, ["a.yaml false", "b.yaml true"]) } @@ -103,7 +113,7 @@ final class WorkflowDiscoveryTests: XCTestCase { let repo = try directory("repo") try write(WorkflowFixtures.minimal(id: "demo"), to: user, name: "demo.yaml") try write(WorkflowFixtures.minimal(id: "demo", extraSteps: " - id: x\n close: ghost"), to: repo, name: "demo.yaml") - let catalog = WorkflowDiscovery.catalog( + let catalog = try WorkflowDiscovery.catalog( sources: WorkflowSources(bundle: nil, user: user, repo: repo), context: context) XCTAssertEqual( catalog.map { "\($0.file.scope.rawValue) valid=\($0.file.isValid) shadowed=\($0.shadowed)" }, diff --git a/ProwlCLITests/WorkflowDocumentParserTests.swift b/ProwlCLITests/WorkflowDocumentParserTests.swift index ec0d6de1..fa4b82ba 100644 --- a/ProwlCLITests/WorkflowDocumentParserTests.swift +++ b/ProwlCLITests/WorkflowDocumentParserTests.swift @@ -107,7 +107,7 @@ final class WorkflowDocumentParserTests: XCTestCase { func testMissingRequiredKeysAndUnsupportedSchema() { XCTAssertEqual(WorkflowFixtures.parseCodes("id: demo\nname: Demo\n"), ["missing_key", "missing_key"]) XCTAssertEqual( - WorkflowFixtures.parseCodes("schema: prowl.workflow/v2\nid: demo\nname: Demo\nsteps: []\n"), + WorkflowFixtures.parseCodes("schema: prowl.workflow/v2\nid: demo\nname: Demo\nsteps:\n - id: a\n notify: hi\n"), ["unsupported_schema"]) } @@ -255,4 +255,35 @@ final class WorkflowDocumentParserTests: XCTestCase { let nested = WorkflowFixtures.minimal(extraSteps: " - id: b\n action: git.context\n with: { root: [a] }") XCTAssertEqual(WorkflowFixtures.parseCodes(nested), ["type_mismatch"]) } + + // MARK: - Round 1 review findings + + func testHugeDurationsDoNotOverflow() { + XCTAssertNil(WorkflowDocumentParser.parseDuration("9223372036854775807h")) + XCTAssertNil(WorkflowDocumentParser.parseDuration("99999999999999999999s")) + XCTAssertEqual(WorkflowDocumentParser.parseDuration("2562047788015215h"), 2562047788015215 * 3600) + let overflow = WorkflowFixtures.minimal( + extraSteps: " - id: b\n message: author\n text: hi\n expect: { timeout: 9223372036854775807h }") + XCTAssertEqual(WorkflowFixtures.parseCodes(overflow), ["timeout_syntax"]) + } + + func testStepsMustNotBeEmpty() { + XCTAssertEqual( + WorkflowFixtures.parseCodes("schema: prowl.workflow/v1\nid: demo\nname: Demo\nsteps: []\n"), ["steps_empty"]) + let emptyBody = WorkflowFixtures.minimal(extraSteps: " - id: loop\n repeat: { max: 2 }\n steps: []") + XCTAssertEqual(WorkflowFixtures.parseCodes(emptyBody), ["steps_empty"]) + } + + func testStringFieldsRejectTypedPlainScalars() { + XCTAssertEqual( + WorkflowFixtures.parseCodes("schema: prowl.workflow/v1\nid: 1\nname: Demo\nsteps:\n - id: a\n notify: hi\n"), + ["type_mismatch"]) + XCTAssertEqual( + WorkflowFixtures.parseCodes("schema: prowl.workflow/v1\nid: demo\nname: 1.5\nsteps:\n - id: a\n notify: hi\n"), + ["type_mismatch"]) + XCTAssertEqual(WorkflowFixtures.parseCodes(WorkflowFixtures.minimal(extraSteps: " - id: b\n notify: true")), ["type_mismatch"]) + XCTAssertEqual(WorkflowFixtures.parseCodes(WorkflowFixtures.minimal(extraSteps: " - id: b\n notify: \"true\"")), []) + XCTAssertEqual(WorkflowFixtures.parseCodes(WorkflowFixtures.minimal(extraSteps: " - id: b\n notify: Round 1")), []) + } } + diff --git a/ProwlCLITests/WorkflowFixtures.swift b/ProwlCLITests/WorkflowFixtures.swift index e1220bf0..ad3ca9ef 100644 --- a/ProwlCLITests/WorkflowFixtures.swift +++ b/ProwlCLITests/WorkflowFixtures.swift @@ -114,10 +114,12 @@ enum WorkflowFixtures { scope: WorkflowScope = .user, bundledSkillIDs: Set? = ["prowl.adversarial-reviewer"], knownAgents: Set? = nil, - installedAgents: Set? = nil + installedAgents: Set? = nil, + enabledProfiles: [WorkflowProfileSuggestion]? = nil ) -> [WorkflowDiagnostic] { let context = WorkflowValidationContext( - scope: scope, bundledSkillIDs: bundledSkillIDs, knownAgents: knownAgents, installedAgents: installedAgents) + scope: scope, bundledSkillIDs: bundledSkillIDs, knownAgents: knownAgents, installedAgents: installedAgents, + enabledProfiles: enabledProfiles) return WorkflowDiscovery.parse(yaml, url: URL(filePath: "/fixture.yaml"), scope: scope, context: context) .diagnostics } @@ -127,10 +129,12 @@ enum WorkflowFixtures { scope: WorkflowScope = .user, bundledSkillIDs: Set? = ["prowl.adversarial-reviewer"], knownAgents: Set? = nil, - installedAgents: Set? = nil + installedAgents: Set? = nil, + enabledProfiles: [WorkflowProfileSuggestion]? = nil ) -> [String] { diagnostics( - yaml, scope: scope, bundledSkillIDs: bundledSkillIDs, knownAgents: knownAgents, installedAgents: installedAgents + yaml, scope: scope, bundledSkillIDs: bundledSkillIDs, knownAgents: knownAgents, installedAgents: installedAgents, + enabledProfiles: enabledProfiles ).map(\.code) } } diff --git a/ProwlCLITests/WorkflowValidatorTests.swift b/ProwlCLITests/WorkflowValidatorTests.swift index c37b6f50..a8198bde 100644 --- a/ProwlCLITests/WorkflowValidatorTests.swift +++ b/ProwlCLITests/WorkflowValidatorTests.swift @@ -289,4 +289,113 @@ final class WorkflowValidatorTests: XCTestCase { XCTAssertEqual(WorkflowFixtures.codes(minimal(roles: role), installedAgents: ["codex"]), []) XCTAssertEqual(WorkflowFixtures.codes(minimal(roles: role)), [], "unknown catalogs skip the warnings") } + + // MARK: - Round 1 review findings + + func testControlCharactersIncludingTabsAreRejectedInTypedText() { + XCTAssertEqual( + WorkflowFixtures.codes(minimal() + "inputs:\n s: { type: string, default: \"has\\ttab\" }\n"), + ["input_default_multiline"]) + XCTAssertEqual( + WorkflowFixtures.codes(minimal(steps: " - id: b\n message: author\n text: \"a\\tb\"")), ["text_multiline"]) + } + + func testRoleInputsOfNativeActionsMustNameDeclaredRoles() { + let missing = " - id: t\n action: handoff.transition\n with: { from: missing, to: also-missing }" + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: missing)), ["unknown_role", "unknown_role"]) + let templated = " - id: t\n action: handoff.transition\n with: { from: \"{{ roles.author.name }}\", to: author }" + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: templated)), ["role_input_literal"]) + let valid = " - id: t\n action: handoff.transition\n with: { from: author, to: author }" + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: valid)), []) + } + + func testVerdictReferencesFollowTheLatestProducer() { + let stale = """ + - id: first + message: author + text: First + expect: { output: result, verdict: [clean, issues] } + - id: second + message: author + text: Second + expect: { output: result } + - id: report + notify: "{{ outputs.result.verdict }}" + """ + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: stale)), ["unknown_variable"]) + let refreshed = """ + - id: first + message: author + text: First + expect: { output: result } + - id: second + message: author + text: Second + expect: { output: result, verdict: [clean, issues] } + - id: report + notify: "{{ outputs.result.verdict }}" + """ + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: refreshed)), []) + } + + func testOutputsProducedOnlyInsideASkippableLoopAreNotVisibleAfterIt() { + let skippable = """ + - id: initial + message: author + text: Initial + expect: { output: verdict, verdict: [clean, issues] } + - id: retry + repeat: { max: 2, until: "outputs.verdict.verdict == clean" } + steps: + - id: produce + message: author + text: Retry + expect: { output: retry_result } + - id: report + notify: "{{ outputs.retry_result.path }}" + """ + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: skippable)), ["unknown_variable"]) + let unconditional = """ + - id: retry + repeat: { max: 2 } + steps: + - id: produce + message: author + text: Retry + expect: { output: retry_result } + - id: report + notify: "{{ outputs.retry_result.path }}" + """ + XCTAssertEqual(WorkflowFixtures.codes(minimal(steps: unconditional)), []) + let produced_before_and_inside = """ + - id: initial + message: author + text: Initial + expect: { output: findings, verdict: [clean, issues] } + - id: loop + repeat: { max: 2, until: "outputs.findings.verdict == clean" } + steps: + - id: again + message: author + text: Again + expect: { output: findings } + - id: path + notify: "{{ outputs.findings.path }}" + - id: verdict + notify: "{{ outputs.findings.verdict }}" + """ + XCTAssertEqual( + WorkflowFixtures.codes(minimal(steps: produced_before_and_inside)), ["until_verdict_undeclared", "unknown_variable"], + "the in-loop producer declares no verdict: until cannot read it and neither can a later reference") + } + + func testSuggestWarnsWhenNoEnabledProfileMatches() { + let role = " r:\n source: launch\n suggest: { agent: codex, reasoning_effort: xhigh }" + let codexHigh = WorkflowProfileSuggestion(agent: "codex", model: "gpt-5", reasoningEffort: "xhigh", executionMode: "standard") + let claude = WorkflowProfileSuggestion(agent: "claude", model: nil, reasoningEffort: nil, executionMode: "standard") + XCTAssertEqual(WorkflowFixtures.codes(minimal(roles: role), enabledProfiles: [claude]), ["suggest_unmatched"]) + XCTAssertEqual(WorkflowFixtures.codes(minimal(roles: role), enabledProfiles: [claude, codexHigh]), []) + XCTAssertEqual(WorkflowFixtures.codes(minimal(roles: role)), [], "no profile catalog: no warning") + } } + diff --git a/supacode/App/supacodeApp.swift b/supacode/App/supacodeApp.swift index 70d2036f..3c30002b 100644 --- a/supacode/App/supacodeApp.swift +++ b/supacode/App/supacodeApp.swift @@ -1175,7 +1175,15 @@ struct SupacodeApp: App { disabledWorkflowIDs: Set(settings.disabledWorkflowIDs), bundledSkillIDs: bundledSkills.map { Set($0.map(\.id)) }, knownAgents: Set(DetectedAgent.allCases.map(\.rawValue)), - installedAgents: installedAgents + installedAgents: installedAgents, + enabledProfiles: settings.agentProfiles.filter(\.isEnabled).map { profile in + WorkflowProfileSuggestion( + agent: profile.runtime.agent.rawValue, + model: profile.model, + reasoningEffort: profile.reasoningEffort, + executionMode: profile.executionMode.rawValue + ) + } ) } return CLICommandRouter( diff --git a/supacode/CLIService/Shared/WorkflowActionRegistry.swift b/supacode/CLIService/Shared/WorkflowActionRegistry.swift index 46957c0b..cd0727ab 100644 --- a/supacode/CLIService/Shared/WorkflowActionRegistry.swift +++ b/supacode/CLIService/Shared/WorkflowActionRegistry.swift @@ -5,13 +5,24 @@ import Foundation nonisolated public struct WorkflowActionInput: Equatable, Sendable { + public enum Kind: String, Equatable, Sendable { + /// Templated text. + case string + /// Templated path. + case path + /// A role name declared by the workflow; never templated. + case role + } + public let name: String public let required: Bool + public let kind: Kind public let description: String - public init(name: String, required: Bool, description: String) { + public init(name: String, required: Bool, kind: Kind = .string, description: String) { self.name = name self.required = required + self.kind = kind self.description = description } } @@ -56,9 +67,10 @@ nonisolated public enum WorkflowActionRegistry { "Archive-first `.prowl/handoff/` transition from one role to another; without a briefing " + "it becomes a context-only transition.", inputs: [ - WorkflowActionInput(name: "briefing", required: false, description: "Path to the validated briefing"), - WorkflowActionInput(name: "from", required: true, description: "Outgoing role"), - WorkflowActionInput(name: "to", required: true, description: "Receiving role"), + WorkflowActionInput( + name: "briefing", required: false, kind: .path, description: "Path to the validated briefing"), + WorkflowActionInput(name: "from", required: true, kind: .role, description: "Outgoing role"), + WorkflowActionInput(name: "to", required: true, kind: .role, description: "Receiving role"), WorkflowActionInput(name: "note", required: false, description: "Log note"), ], outputs: [ @@ -71,7 +83,8 @@ nonisolated public enum WorkflowActionRegistry { id: "handoff.checkpoint", description: "Save progress for a later successor; regenerates `context.md`.", inputs: [ - WorkflowActionInput(name: "briefing", required: false, description: "Path to the validated briefing"), + WorkflowActionInput( + name: "briefing", required: false, kind: .path, description: "Path to the validated briefing"), WorkflowActionInput(name: "note", required: false, description: "Log note"), ], outputs: [ @@ -83,7 +96,8 @@ nonisolated public enum WorkflowActionRegistry { id: "git.context", description: "Generate a markdown summary of the worktree's repository state.", inputs: [ - WorkflowActionInput(name: "root", required: false, description: "Repository root; defaults to the worktree") + WorkflowActionInput( + name: "root", required: false, kind: .path, description: "Repository root; defaults to the worktree") ], outputs: [ WorkflowActionOutput(name: "path", description: "Path to the generated markdown summary"), diff --git a/supacode/CLIService/Shared/WorkflowDiscovery.swift b/supacode/CLIService/Shared/WorkflowDiscovery.swift index df3473bb..2c4ac79d 100644 --- a/supacode/CLIService/Shared/WorkflowDiscovery.swift +++ b/supacode/CLIService/Shared/WorkflowDiscovery.swift @@ -69,16 +69,16 @@ nonisolated public enum WorkflowDiscovery { public static let fileExtensions: Set = ["yaml", "yml"] /// Parses and validates every workflow file directly inside `directory`, in file-name order. + /// A missing directory is an empty source; one that exists but cannot be read throws. public static func files( in directory: URL?, scope: WorkflowScope, context: WorkflowValidationContext, fileManager: FileManager = .default - ) -> [WorkflowSourceFile] { - guard let directory else { return [] } - let contents = - (try? fileManager.contentsOfDirectory( - at: directory, includingPropertiesForKeys: [.isRegularFileKey], options: [.skipsHiddenFiles])) ?? [] + ) throws -> [WorkflowSourceFile] { + guard let directory, fileManager.fileExists(atPath: directory.path(percentEncoded: false)) else { return [] } + let contents = try fileManager.contentsOfDirectory( + at: directory, includingPropertiesForKeys: [.isRegularFileKey], options: [.skipsHiddenFiles]) return contents .filter { fileExtensions.contains($0.pathExtension.lowercased()) } @@ -119,9 +119,9 @@ nonisolated public enum WorkflowDiscovery { sources: WorkflowSources, context: (WorkflowScope) -> WorkflowValidationContext, fileManager: FileManager = .default - ) -> [WorkflowCatalogEntry] { + ) throws -> [WorkflowCatalogEntry] { let files = - files(in: sources.bundle, scope: .bundle, context: context(.bundle), fileManager: fileManager) + try files(in: sources.bundle, scope: .bundle, context: context(.bundle), fileManager: fileManager) + files(in: sources.user, scope: .user, context: context(.user), fileManager: fileManager) + files(in: sources.repo, scope: .repo, context: context(.repo), fileManager: fileManager) var winners: [String: URL] = [:] diff --git a/supacode/CLIService/Shared/WorkflowDocumentParser.swift b/supacode/CLIService/Shared/WorkflowDocumentParser.swift index 44e3549d..3e7c0120 100644 --- a/supacode/CLIService/Shared/WorkflowDocumentParser.swift +++ b/supacode/CLIService/Shared/WorkflowDocumentParser.swift @@ -51,7 +51,8 @@ nonisolated public enum WorkflowDocumentParser { let name = document.requiredString("name") let inputs = document.mapping("inputs").map(parseInputs) ?? [] let roles = document.mapping("roles").map(parseRoles) ?? [] - let steps = document.requiredSequence("steps").map { parseSteps($0, insideRepeat: false) } ?? [] + let steps = + document.requiredSequence("steps").map { parseSteps($0, insideRepeat: false, at: document.location) } ?? [] guard let id, let name else { return nil } return WorkflowDefinition( id: id, @@ -170,8 +171,13 @@ nonisolated public enum WorkflowDocumentParser { private static let verbKeys = ["message", "launch", "action", "notify", "close", "repeat"] - private static func parseSteps(_ steps: SequenceReader, insideRepeat: Bool) -> [WorkflowStepDefinition] { - steps.mappings().compactMap { parseStep($0, insideRepeat: insideRepeat) } + private static func parseSteps( + _ steps: SequenceReader, insideRepeat: Bool, at location: WorkflowSourceLocation? + ) -> [WorkflowStepDefinition] { + if steps.isEmpty { + steps.collector.error("steps_empty", "'\(steps.path)' needs at least one step.", at: location) + } + return steps.mappings().compactMap { parseStep($0, insideRepeat: insideRepeat) } } private static func parseStep(_ step: MappingReader, insideRepeat: Bool) -> WorkflowStepDefinition? { @@ -277,7 +283,7 @@ nonisolated public enum WorkflowDocumentParser { body.checkKeys(["max", "until"]) let max = parseRepeatBound(body) let until = body.string("until").flatMap { parseUntil($0, at: body.location(of: "until"), body.collector) } - let steps = step.requiredSequence("steps").map { parseSteps($0, insideRepeat: true) } + let steps = step.requiredSequence("steps").map { parseSteps($0, insideRepeat: true, at: step.location) } guard let max, let steps else { return nil } return .repeat(max: max, until: until, steps: steps) } @@ -360,11 +366,14 @@ nonisolated public enum WorkflowDocumentParser { guard let match = text.trimmingCharacters(in: .whitespaces).wholeMatch(of: /^(\d+)\s*([smh])$/), let amount = Int(match.1) else { return nil } - switch match.2 { - case "s": return amount - case "m": return amount * 60 - default: return amount * 3600 - } + let factor = + switch match.2 { + case "s": 1 + case "m": 60 + default: 3600 + } + let (seconds, overflow) = amount.multipliedReportingOverflow(by: factor) + return overflow ? nil : seconds } } @@ -462,8 +471,20 @@ nonisolated struct MappingReader { } } + /// A string-valued field. Unquoted scalars that YAML resolves to a number, boolean, or null are + /// type errors so that `id: 1` cannot masquerade as the string "1" (quote it to keep text). func string(_ key: String) -> String? { guard let node = mapping[key] else { return nil } + return strictText(node, key: key) + } + + func strictText(_ node: Node, key: String) -> String? { + if node.isPlainScalar, node.int != nil || node.bool != nil || node.float != nil { + collector.error( + "type_mismatch", "'\(key)' in \(path) must be a string; quote the value to keep it as text.", + at: node.sourceLocation) + return nil + } return scalarText(node, key: key) } @@ -527,7 +548,7 @@ nonisolated struct MappingReader { collector.error("type_mismatch", "'\(key)' in \(path) must be a list.", at: node.sourceLocation) return nil } - return sequence.compactMap { scalarText($0, key: key) } + return sequence.compactMap { strictText($0, key: key) } } func mapping(_ key: String) -> MappingReader? { @@ -558,6 +579,8 @@ nonisolated struct SequenceReader { self.path = path } + var isEmpty: Bool { sequence.isEmpty } + func mappings() -> [MappingReader] { sequence.enumerated().compactMap { index, node in MappingReader(node: node, collector: collector, path: "\(path)[\(index)]") diff --git a/supacode/CLIService/Shared/WorkflowValidator.swift b/supacode/CLIService/Shared/WorkflowValidator.swift index 2a0353a5..9a476942 100644 --- a/supacode/CLIService/Shared/WorkflowValidator.swift +++ b/supacode/CLIService/Shared/WorkflowValidator.swift @@ -18,6 +18,8 @@ nonisolated public struct WorkflowValidationContext: Sendable { public let knownAgents: Set? /// Agents installed locally; nil = the "nothing installed" warning is skipped. public let installedAgents: Set? + /// Preset fields of the enabled Agent Profiles; nil = the `suggest` match warning is skipped. + public let enabledProfiles: [WorkflowProfileSuggestion]? public let actions: [WorkflowActionSchema] public init( @@ -25,12 +27,14 @@ nonisolated public struct WorkflowValidationContext: Sendable { bundledSkillIDs: Set? = nil, knownAgents: Set? = nil, installedAgents: Set? = nil, + enabledProfiles: [WorkflowProfileSuggestion]? = nil, actions: [WorkflowActionSchema] = WorkflowActionRegistry.all ) { self.scope = scope self.bundledSkillIDs = bundledSkillIDs self.knownAgents = knownAgents self.installedAgents = installedAgents + self.enabledProfiles = enabledProfiles self.actions = actions } } @@ -49,8 +53,7 @@ nonisolated public enum WorkflowValidator { static func isSingleLine(_ text: String) -> Bool { !text.unicodeScalars.contains { scalar in - scalar == "\n" || scalar == "\r" || scalar == "\u{2028}" || scalar == "\u{2029}" - || (scalar.value < 0x20 && scalar != "\t") || (0x7F...0x9F).contains(scalar.value) + scalar == "\u{2028}" || scalar == "\u{2029}" || scalar.value < 0x20 || (0x7F...0x9F).contains(scalar.value) } } } @@ -64,8 +67,17 @@ nonisolated private enum OutputConsumer { case optionalActionInput } +nonisolated private struct OutputProducer { + let verdicts: Set? + /// The `repeat` step whose body holds the producer; nil at the top level. + let loopID: String? +} + nonisolated private struct OutputInfo { - var verdicts: Set? + var producers: [OutputProducer] = [] + /// Verdict set of the producer whose delivery is the latest at this point of the walk; nil + /// when it declares none, or when a skippable loop leaves the latest producer ambiguous. + var latestVerdicts: Set? var skipLocations: [WorkflowSourceLocation?] = [] } @@ -81,6 +93,7 @@ nonisolated private final class Walker { /// Action steps visible to the step being validated: outer sequences first, current last. private var actionScopes: [[String: WorkflowActionSchema]] = [[:]] private var insideRepeat = false + private var currentLoopID: String? private var loopSeen = false init(definition: WorkflowDefinition, context: WorkflowValidationContext) { @@ -166,9 +179,29 @@ nonisolated private final class Walker { "No installed agent satisfies role '\(role.name)' (\(agents.joined(separator: ", "))).", at: role.location) } + if let suggest = launch.suggest, let profiles = context.enabledProfiles, + !profiles.contains(where: { Self.profile($0, matches: suggest) }) + { + collector.warning( + "suggest_unmatched", "No enabled Agent Profile matches the suggestion for role '\(role.name)'.", + at: role.location) + } } } + /// Every field the suggestion names must equal the profile's; absent fields do not constrain. + private static func profile( + _ profile: WorkflowProfileSuggestion, matches suggest: WorkflowProfileSuggestion + ) -> Bool { + for (wanted, actual) in [ + (suggest.agent, profile.agent), (suggest.model, profile.model), + (suggest.reasoningEffort, profile.reasoningEffort), (suggest.executionMode, profile.executionMode), + ] where wanted != nil && wanted != actual { + return false + } + return true + } + private func checkAgentTokens(_ tokens: [String], role: WorkflowRoleDefinition) { guard let known = context.knownAgents else { return } for token in tokens where !known.contains(token) { @@ -270,7 +303,19 @@ nonisolated private final class Walker { collector.error("unknown_action_input", "Action '\(id)' has no input '\(key)'.", at: step.location) continue } - checkTemplate(value, at: step.location, consumer: input.required ? .requiredActionInput : .optionalActionInput) + switch input.kind { + case .role: + if WorkflowTemplate.containsReference(value) { + collector.error( + "role_input_literal", "Action '\(id)' input '\(key)' must name a role literally, not a template.", + at: step.location) + } else if definition.role(named: value) == nil { + collector.error( + "unknown_role", "Action '\(id)' input '\(key)' names undefined role '\(value)'.", at: step.location) + } + case .string, .path: + checkTemplate(value, at: step.location, consumer: input.required ? .requiredActionInput : .optionalActionInput) + } } for input in schema.inputs where input.required && inputs[input.name] == nil { collector.error("missing_action_input", "Action '\(id)' requires input '\(input.name)'.", at: step.location) @@ -283,15 +328,36 @@ nonisolated private final class Walker { body: [WorkflowStepDefinition] ) { checkRepeatBound(max, at: step.location) - let producedBefore = Set(outputs.keys) + let before = outputs insideRepeat = true + currentLoopID = step.id actionScopes.append([:]) body.forEach(checkStep) actionScopes.removeLast() insideRepeat = false + currentLoopID = nil loopSeen = true - if let until { - checkUntil(until, producedBefore: producedBefore, at: until.location ?? step.location) + guard let until else { return } + checkUntil(until, loopID: step.id, before: before, at: until.location ?? step.location) + foldSkippableLoopOutputs(before: before) + } + + /// A loop with `until` may run zero times: outputs first produced inside it are not visible + /// afterwards, and an output also produced before keeps only the verdicts both producers + /// declare. + private func foldSkippableLoopOutputs(before: [String: OutputInfo]) { + for (name, info) in outputs { + guard let earlier = before[name] else { + outputs[name] = nil + continue + } + var folded = info + if let outer = earlier.latestVerdicts, let inner = info.latestVerdicts { + folded.latestVerdicts = outer.intersection(inner) + } else { + folded.latestVerdicts = nil + } + outputs[name] = folded } } @@ -314,8 +380,11 @@ nonisolated private final class Walker { } } + /// `until` reads the latest delivery of its output: before entry that is the last producer + /// before the loop, after each iteration any producer in the body — every one of them must + /// declare a verdict set that holds the literals. private func checkUntil( - _ until: WorkflowUntilCondition, producedBefore: Set, at location: WorkflowSourceLocation? + _ until: WorkflowUntilCondition, loopID: String, before: [String: OutputInfo], at location: WorkflowSourceLocation? ) { guard let info = outputs[until.output] else { collector.error( @@ -323,11 +392,16 @@ nonisolated private final class Walker { at: location) return } - _ = producedBefore - guard let verdicts = info.verdicts else { + var relevant = info.producers.filter { $0.loopID == loopID } + if let last = before[until.output]?.producers.last { + relevant.append(last) + } + let sets = relevant.compactMap(\.verdicts) + guard sets.count == relevant.count, let first = sets.first else { collector.error("until_verdict_undeclared", "Output '\(until.output)' declares no verdict.", at: location) return } + let verdicts = sets.dropFirst().reduce(first) { $0.intersection($1) } if until.values.isEmpty { collector.error("until_syntax", "'until' needs at least one verdict value.", at: location) } @@ -356,9 +430,9 @@ nonisolated private final class Walker { collector.warning("timeout_long", "'timeout' above 2h; the watchdog already supervises waiting.", at: location) } var info = outputs[name] ?? OutputInfo() - if let verdict = expect.verdict { - info.verdicts = (info.verdicts ?? []).union(verdict) - } + let verdicts = expect.verdict.map(Set.init) + info.producers.append(OutputProducer(verdicts: verdicts, loopID: currentLoopID)) + info.latestVerdicts = verdicts if expect.onTimeout == .skip { info.skipLocations.append(location) } @@ -440,7 +514,7 @@ nonisolated private final class Walker { _ parts: [String], at location: WorkflowSourceLocation?, consumer: OutputConsumer ) -> Bool { guard let info = outputs[parts[1]], ["path", "verdict"].contains(parts[2]) else { return false } - if parts[2] == "verdict", info.verdicts == nil { + if parts[2] == "verdict", info.latestVerdicts == nil { return false } consumers[parts[1], default: []].append(consumer) diff --git a/supacode/CLIService/WorkflowCommandHandler.swift b/supacode/CLIService/WorkflowCommandHandler.swift index dfd32e2c..4cea7c40 100644 --- a/supacode/CLIService/WorkflowCommandHandler.swift +++ b/supacode/CLIService/WorkflowCommandHandler.swift @@ -14,6 +14,8 @@ struct WorkflowRuntimeSnapshot { let bundledSkillIDs: Set? let knownAgents: Set let installedAgents: Set? + /// Preset fields of the enabled Agent Profiles, for the `suggest` match warning. + let enabledProfiles: [WorkflowProfileSuggestion] } @MainActor @@ -104,12 +106,13 @@ final class WorkflowCommandHandler: CommandHandler { WorkflowSources.repoDirectory(root: URL(filePath: $0.rootPath, directoryHint: .isDirectory)) } let sources = WorkflowSources(bundle: snapshot.bundleWorkflowsURL, user: snapshot.userWorkflowsURL, repo: repoURL) - let catalog = WorkflowDiscovery.catalog(sources: sources) { scope in + let catalog = try WorkflowDiscovery.catalog(sources: sources) { scope in WorkflowValidationContext( scope: scope, bundledSkillIDs: snapshot.bundledSkillIDs, knownAgents: snapshot.knownAgents, - installedAgents: snapshot.installedAgents + installedAgents: snapshot.installedAgents, + enabledProfiles: snapshot.enabledProfiles ) } let workflows = catalog.map { entry in diff --git a/supacodeTests/WorkflowCommandHandlerTests.swift b/supacodeTests/WorkflowCommandHandlerTests.swift index 28cc71d0..50ac7585 100644 --- a/supacodeTests/WorkflowCommandHandlerTests.swift +++ b/supacodeTests/WorkflowCommandHandlerTests.swift @@ -74,7 +74,8 @@ struct WorkflowCommandHandlerTests { disabledWorkflowIDs: disabled, bundledSkillIDs: [], knownAgents: ["codex", "claude"], - installedAgents: nil + installedAgents: nil, + enabledProfiles: [] ) } } @@ -145,6 +146,18 @@ struct WorkflowCommandHandlerTests { #expect(missing.error?.code == CLIErrorCode.targetNotFound) } + @Test func unreadableSourceDirectoriesFailWithWorkflowFailed() async throws { + let fixture = try Fixture() + defer { fixture.cleanUp() } + let path = fixture.userWorkflows.path(percentEncoded: false) + try FileManager.default.setAttributes([.posixPermissions: 0o000], ofItemAtPath: path) + defer { try? FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: path) } + let handler = WorkflowCommandHandler { fixture.snapshot(focused: true) } + let (response, _) = try await list(handler) + #expect(response.ok == false) + #expect(response.error?.code == CLIErrorCode.workflowFailed) + } + @Test func disabledKeysCombineScopeAndID() { #expect(WorkflowCommandHandler.disabledKey(scope: .repo, id: "review") == "repo/review") } -- 2.51.2