fix(config): install sheet silently closed after Continue in config step

Two bugs chained into the observed "install completed but project
didn't show up" report. Either one would have been enough on its own;
both are here so both are fixed.

Primary bug: TemplateConfigSheet's Cancel + Continue buttons each
called `@Environment(\.dismiss)` after their state-update callbacks.
That was fine when the sheet is presented standalone (the post-install
Configuration button uses it this way and wants dismissal), but Phase C
also INLINED the same view inside TemplateInstallSheet.configureView
for the install flow's .awaitingConfig stage — there's no intermediate
.sheet() presenter there, so `dismiss()` resolved to the OUTER install
sheet. Clicking Continue → configure form's `onCommit` fired
`installerViewModel.submitConfig(values:)` which advanced stage to
.planned, then the dismiss() closed the whole install sheet before
the preview ever rendered. install() was never called.

Fix: remove both dismiss() calls from TemplateConfigSheet. Dismissal
is now the caller's responsibility. ConfigEditorSheet (standalone
mode) already calls `dismiss()` inside its own onCancel closure and
lets the .succeeded state's Done button handle commit-dismissal, so
nothing breaks there. The install flow's state machine advances to
the preview stage where the existing Install/Cancel buttons drive
everything from there.

Secondary bug (latent, same class): ProjectDashboardService.saveRegistry
swallowed both directory-creation and file-write errors with `try?`.
If the `~/.hermes/scarf/` dir creation or projects.json write ever
failed for any reason (permissions, readonly filesystem, sandbox),
the installer's registerProject returned a valid-looking ProjectEntry
while the registry on disk never received the row. Same symptom
surface as the primary bug: install "succeeds," project invisible.

Fix: saveRegistry now throws. Updated all four callers:
- ProjectTemplateInstaller.registerProject: `try` — a registry
  write failure aborts install with a user-visible failure screen.
  This is the critical path; silent success on a destructive op is
  the exact failure mode we want to eliminate.
- ProjectTemplateUninstaller: `do/catch` + logger.warning — we're at
  the final step of uninstall after every other side effect has
  already completed (files removed, skills removed, cron removed,
  memory stripped, Keychain cleared). Leaving a stale registry row
  pointing at a deleted project is cosmetic and easy to fix from
  the sidebar minus button.
- ProjectsViewModel.addProject + removeProject: `do/catch` +
  logger.error. The VM doesn't currently have a surface for
  user-visible errors (no toast/alert on this view), but the
  failure now at least lands in the unified log instead of
  disappearing. Proper in-UI error surface is tracked as follow-up.
- ProjectDashboardService.loadRegistry: switched its stale `print`
  to `logger.error` while I was in the file.

Tests: added TemplateInstallerViewModelTests suite (3 tests) covering
the install VM's configure-step state transitions:
- submitConfigStashesValuesAndTransitionsToPlanned — .awaitingConfig
  → .planned + configValues stash on the plan. The exact transition
  that the dismiss() bug tore down mid-flight.
- cancelConfigReturnsToAwaitingParentDirectory — back-button behaviour
  with plan preserved so re-entry doesn't re-run buildPlan.
- submitConfigNoOpWhenPlanIsNil — defensive guard.

These won't catch a view-level regression (Swift Testing doesn't do
UI tests in this project), but they lock in the VM state-machine
contract so the next refactor can't silently break submitConfig or
cancelConfig without failing CI.

53/53 Swift tests + 24/24 Python tests + catalog validator clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Alan Wizemann
2026-04-23 03:26:17 +02:00
parent 81e8da91d6
commit 3bd95de8f4
7 changed files with 261 additions and 17 deletions
@@ -1,6 +1,8 @@
import Foundation
import os
struct ProjectDashboardService: Sendable {
private static let logger = Logger(subsystem: "com.scarf", category: "ProjectDashboardService")
let context: ServerContext
let transport: any ServerTransport
@@ -19,23 +21,28 @@ struct ProjectDashboardService: Sendable {
do {
return try JSONDecoder().decode(ProjectRegistry.self, from: data)
} catch {
print("[Scarf] Failed to decode project registry: \(error.localizedDescription)")
Self.logger.error("Failed to decode project registry: \(error.localizedDescription, privacy: .public)")
return ProjectRegistry(projects: [])
}
}
func saveRegistry(_ registry: ProjectRegistry) {
/// Persist the project registry to `~/.hermes/scarf/projects.json`.
///
/// **Throws** on every non-success path the previous version of
/// this method silently swallowed `createDirectory` and `writeFile`
/// failures with `try?`, which meant the installer could return a
/// valid-looking `ProjectEntry` while the registry on disk never
/// received the new row (project would complete install, show a
/// success screen, then be invisible in the sidebar). Callers that
/// want fire-and-forget behaviour can still use `try?`, but the
/// choice is now theirs.
func saveRegistry(_ registry: ProjectRegistry) throws {
let dir = context.paths.scarfDir
if !transport.fileExists(dir) {
do {
try transport.createDirectory(dir)
} catch {
print("[Scarf] Failed to create scarf directory: \(error.localizedDescription)")
return
}
}
guard let data = try? JSONEncoder().encode(registry) else { return }
// Pretty-print for readability (agents may read this file)
let data = try JSONEncoder().encode(registry)
// Pretty-print for readability (agents may read this file).
let writeData: Data
if let pretty = try? JSONSerialization.jsonObject(with: data),
let formatted = try? JSONSerialization.data(withJSONObject: pretty, options: [.prettyPrinted, .sortedKeys]) {
@@ -43,7 +50,7 @@ struct ProjectDashboardService: Sendable {
} else {
writeData = data
}
try? transport.writeFile(context.paths.projectsRegistry, data: writeData)
try transport.writeFile(context.paths.projectsRegistry, data: writeData)
}
// MARK: - Dashboard
@@ -211,7 +211,13 @@ struct ProjectTemplateInstaller: Sendable {
var registry = service.loadRegistry()
let entry = ProjectEntry(name: plan.projectRegistryName, path: plan.projectDir)
registry.projects.append(entry)
service.saveRegistry(registry)
// Must throw on failure silent failure here used to make the
// installer return a valid entry while the registry on disk
// never got updated, producing the "install completed but the
// project doesn't show up in the sidebar" bug. If the registry
// write fails, the whole install is surfaced as failed so the
// user can see + address the underlying problem.
try service.saveRegistry(registry)
return entry
}
@@ -206,7 +206,17 @@ struct ProjectTemplateUninstaller: Sendable {
let dashboardService = ProjectDashboardService(context: context)
var registry = dashboardService.loadRegistry()
registry.projects.removeAll { $0.path == plan.project.path }
dashboardService.saveRegistry(registry)
// saveRegistry throws now log a write failure but don't abort
// the uninstall. Every earlier step already completed (files
// removed, skills removed, cron jobs removed, memory stripped,
// Keychain cleared); failing here leaves a stale registry row
// pointing at a deleted project cosmetic and easy to fix
// from the sidebar.
do {
try dashboardService.saveRegistry(registry)
} catch {
Self.logger.warning("uninstall couldn't rewrite projects registry: \(error.localizedDescription, privacy: .public)")
}
Self.logger.info("uninstalled template \(plan.lock.templateId, privacy: .public) from \(plan.project.path, privacy: .public)")
}
@@ -1,7 +1,9 @@
import Foundation
import os
@Observable
final class ProjectsViewModel {
private let logger = Logger(subsystem: "com.scarf", category: "ProjectsViewModel")
let context: ServerContext
private let service: ProjectDashboardService
@@ -39,7 +41,19 @@ final class ProjectsViewModel {
guard !registry.projects.contains(where: { $0.name == name }) else { return }
let entry = ProjectEntry(name: name, path: path)
registry.projects.append(entry)
service.saveRegistry(registry)
// saveRegistry throws now. The VM doesn't currently have a
// surface for user-visible errors (there's no alert/toast in
// the Projects view), so log at error level to the unified
// log and keep the in-memory state consistent with whatever
// landed on disk. If the write fails, the added entry won't
// persist across launches the user sees it appear + work
// this session, then it's gone at relaunch. Not ideal, but
// matches today's UX and flagged for a proper alert later.
do {
try service.saveRegistry(registry)
} catch {
logger.error("addProject couldn't persist registry: \(error.localizedDescription, privacy: .public)")
}
projects = registry.projects
selectProject(entry)
}
@@ -47,7 +61,11 @@ final class ProjectsViewModel {
func removeProject(_ project: ProjectEntry) {
var registry = service.loadRegistry()
registry.projects.removeAll { $0.name == project.name }
service.saveRegistry(registry)
do {
try service.saveRegistry(registry)
} catch {
logger.error("removeProject couldn't persist registry: \(error.localizedDescription, privacy: .public)")
}
projects = registry.projects
if selectedProject?.name == project.name {
selectedProject = nil
@@ -68,16 +68,26 @@ struct TemplateConfigSheet: View {
private var footer: some View {
HStack {
Button("Cancel") {
// Caller owns dismissal this view is used both as a
// standalone sheet (ConfigEditorSheet, where the caller
// wants dismissal) AND inlined inside the install sheet
// (TemplateInstallSheet.configureView, where calling
// .dismiss here would tear down the OUTER install sheet
// and abort the flow before .planned is reached).
onCancel()
dismiss()
}
.keyboardShortcut(.cancelAction)
Spacer()
Button(commitLabel) {
if let finalized = viewModel.commit(project: project) {
onCommit(finalized)
dismiss()
}
// Same dismissal-is-caller's-responsibility rule as
// Cancel inside the install sheet, onCommit transitions
// stage to .planned and the outer view re-renders to
// show the preview. In the edit sheet, onCommit
// transitions the editor VM and its state machine
// handles dismissal via the success view's Done button.
}
.keyboardShortcut(.defaultAction)
.buttonStyle(.borderedProminent)
+88
View File
@@ -49,6 +49,10 @@
},
"(%lld tokens)" : {
},
"*" : {
"comment" : "A required asterisk.",
"isCommentAutoGenerated" : true
},
"/%@" : {
@@ -2229,6 +2233,9 @@
"already gone" : {
"comment" : "A tag for a file that is already gone (no longer in the template).",
"isCommentAutoGenerated" : true
},
"Also works: %@" : {
},
"API Key" : {
"localizations" : {
@@ -5024,6 +5031,14 @@
}
}
},
"Configuration saved" : {
"comment" : "A title displayed when a configuration is saved.",
"isCommentAutoGenerated" : true
},
"Configuration…" : {
"comment" : "A contextual menu item that opens a configuration editor for a project.",
"isCommentAutoGenerated" : true
},
"Configure" : {
"localizations" : {
"de" : {
@@ -5064,6 +5079,10 @@
}
}
},
"Configure %@" : {
"comment" : "The title of the configuration sheet. The argument is the name of the template.",
"isCommentAutoGenerated" : true
},
"Connect timeout" : {
"localizations" : {
"de" : {
@@ -5304,6 +5323,10 @@
}
}
},
"Continue" : {
"comment" : "Button label for continuing with the template configuration.",
"isCommentAutoGenerated" : true
},
"Continue Last Session" : {
"localizations" : {
"de" : {
@@ -5584,6 +5607,10 @@
}
}
},
"Couldn't save" : {
"comment" : "A title displayed when a configuration save fails.",
"isCommentAutoGenerated" : true
},
"Create" : {
"localizations" : {
"de" : {
@@ -7657,6 +7684,10 @@
}
}
},
"Edit configuration" : {
"comment" : "A button that opens a configuration editor for a project.",
"isCommentAutoGenerated" : true
},
"Edit User Profile" : {
"localizations" : {
"de" : {
@@ -10548,6 +10579,9 @@
}
}
}
},
"Internal state inconsistency — please close and re-open." : {
},
"Invalid URL" : {
"localizations" : {
@@ -11156,6 +11190,10 @@
}
}
},
"Loading configuration…" : {
"comment" : "A message displayed while loading the configuration.",
"isCommentAutoGenerated" : true
},
"Loading session…" : {
"localizations" : {
"de" : {
@@ -12665,6 +12703,9 @@
}
}
}
},
"No configuration" : {
},
"No credential pools configured" : {
"localizations" : {
@@ -12910,6 +12951,10 @@
}
}
},
"No fields" : {
"comment" : "A label that describes a template with no configuration fields.",
"isCommentAutoGenerated" : true
},
"No headers configured." : {
"localizations" : {
"de" : {
@@ -14258,6 +14303,10 @@
}
}
},
"Opens on launch" : {
"comment" : "A tooltip for the star button in the Manage Servers view.",
"isCommentAutoGenerated" : true
},
"Optional" : {
"localizations" : {
"de" : {
@@ -16127,6 +16176,10 @@
}
}
},
"Recommended model" : {
"comment" : "A label that indicates a recommended model.",
"isCommentAutoGenerated" : true
},
"Reconnect" : {
"localizations" : {
"de" : {
@@ -18000,6 +18053,14 @@
}
}
},
"Saved in Keychain — leave empty to keep the stored value." : {
"comment" : "A message that appears when a user has filled in a secret but has not yet saved it.",
"isCommentAutoGenerated" : true
},
"Saving…" : {
"comment" : "A label displayed while the configuration is being saved.",
"isCommentAutoGenerated" : true
},
"Scarf" : {
},
@@ -18043,6 +18104,10 @@
}
}
},
"Scarf doesn't auto-switch your active model. Change it in Settings if you'd like." : {
"comment" : "A description of the warning about not switching models.",
"isCommentAutoGenerated" : true
},
"Scarf never prompts for passphrases. Add your key to ssh-agent in Terminal, then click Retry. If your key isn't `id_ed25519`, swap the path:" : {
"localizations" : {
"de" : {
@@ -19291,6 +19356,10 @@
}
}
},
"Set as default — open this server when Scarf launches." : {
"comment" : "A tooltip for the star button in the Manage Servers view.",
"isCommentAutoGenerated" : true
},
"Settings" : {
"localizations" : {
"de" : {
@@ -19694,6 +19763,10 @@
}
}
},
"Show while typing" : {
"comment" : "A hint for the user on how to show/hide the secret.",
"isCommentAutoGenerated" : true
},
"Signal integration requires signal-cli (Java-based) installed locally. Link this Mac as a Signal device, then keep the daemon running so hermes can send/receive messages." : {
"localizations" : {
"de" : {
@@ -21538,6 +21611,13 @@
}
}
}
},
"This Mac" : {
"comment" : "A description of the local machine.",
"isCommentAutoGenerated" : true
},
"This project wasn't installed from a schemaful template." : {
},
"This provider has no catalogued models." : {
"localizations" : {
@@ -21739,6 +21819,10 @@
}
}
},
"This template has no configuration fields." : {
"comment" : "A description of a template with no configuration fields.",
"isCommentAutoGenerated" : true
},
"This uploads logs, config (with secrets redacted), and system info to Nous Research support infrastructure. Review the output below before sharing the returned URL." : {
"localizations" : {
"de" : {
@@ -23786,6 +23870,10 @@
},
"Where should this project live?" : {
},
"Will be saved to the Keychain on commit." : {
"comment" : "A description of a secret field that will be saved to the Keychain on commit.",
"isCommentAutoGenerated" : true
},
"Working" : {
"localizations" : {
+105
View File
@@ -768,6 +768,111 @@ import Foundation
}
}
/// State-machine tests for `TemplateInstallerViewModel`. The install
/// flow's configure step is driven entirely through the VM the view
/// transitions `.awaitingParentDirectory .awaitingConfig .planned`
/// based on `submitConfig(values:)` / `cancelConfig()` calls. If those
/// transitions break, the user lands on the wrong sheet stage (or no
/// sheet at all, as in the v1.1.0 regression where the config sheet's
/// internal `dismiss()` tore down the outer install sheet before
/// submitConfig had a chance to fire).
@Suite(.serialized) @MainActor struct TemplateInstallerViewModelTests {
@Test func submitConfigStashesValuesAndTransitionsToPlanned() throws {
let vm = TemplateInstallerViewModel(context: .local)
// Seed the VM with an awaiting-config plan (schema-ful).
let plan = try Self.makePlanWithConfigSchema()
vm.plan = plan
vm.stage = .awaitingConfig
let values: [String: TemplateConfigValue] = [
"site_url": .string("https://example.com")
]
vm.submitConfig(values: values)
// Stage must advance past the configure step, values must land
// on the plan where install() will pick them up.
if case .planned = vm.stage {
// ok
} else {
Issue.record("expected .planned, got \(vm.stage)")
}
#expect(vm.plan?.configValues["site_url"] == .string("https://example.com"))
}
@Test func cancelConfigReturnsToAwaitingParentDirectory() throws {
let vm = TemplateInstallerViewModel(context: .local)
vm.plan = try Self.makePlanWithConfigSchema()
vm.stage = .awaitingConfig
vm.cancelConfig()
if case .awaitingParentDirectory = vm.stage {
// ok user can re-pick the parent dir or fully cancel
} else {
Issue.record("expected .awaitingParentDirectory, got \(vm.stage)")
}
// Plan is preserved so re-entering the configure step doesn't
// re-run buildPlan.
#expect(vm.plan != nil)
}
@Test func submitConfigNoOpWhenPlanIsNil() {
let vm = TemplateInstallerViewModel(context: .local)
vm.plan = nil
vm.stage = .awaitingConfig
vm.submitConfig(values: ["k": .string("v")])
// With no plan, the call should be silent no crash, stage
// stays where it was. (Defensive guard in submitConfig.)
if case .awaitingConfig = vm.stage {
// ok
} else {
Issue.record("expected stage to remain .awaitingConfig when plan is nil; got \(vm.stage)")
}
}
// MARK: - Fixture
/// Build a `TemplateInstallPlan` carrying a single-field config
/// schema. Exists as a local helper rather than a shared one
/// because no other suite needs it.
nonisolated static func makePlanWithConfigSchema() throws -> TemplateInstallPlan {
let schema = TemplateConfigSchema(
fields: [
.init(key: "site_url", type: .string, label: "Site URL",
description: nil, required: true, placeholder: nil,
defaultValue: nil, options: nil, minLength: nil,
maxLength: nil, pattern: nil, minNumber: nil,
maxNumber: nil, step: nil, itemType: nil,
minItems: nil, maxItems: nil)
],
modelRecommendation: nil
)
let manifest = ProjectTemplateServiceTests.sampleManifest(
id: "tester/vm-transitions",
configSchema: schema
)
let tmp = try ProjectTemplateServiceTests.makeTempDir()
// Not a real bundle dir we never unzip or install from this
// plan, we only test state transitions that don't touch disk.
return TemplateInstallPlan(
manifest: manifest,
unpackedDir: tmp,
projectDir: tmp + "/project",
projectFiles: [],
skillsNamespaceDir: nil,
skillsFiles: [],
cronJobs: [],
memoryAppendix: nil,
memoryPath: ServerContext.local.paths.memoryMD,
projectRegistryName: "VM Transitions",
configSchema: schema,
configValues: [:],
manifestCachePath: tmp + "/project/.scarf/manifest.json"
)
}
}
/// Validates every `.scarftemplate` shipped under `templates/<author>/<name>/`
/// in the repo. A template whose manifest, `contents` claim, or file set is
/// out of sync will fail here so shipped templates can't silently rot.