Context
Found during a real end-to-end Linux container test before merging PR #10: `lazygit` isn't packaged in Ubuntu's default apt repos, which caused `Package`'s install step to fail. That specific problem is fixed (see the `installAll` change in `engine/kinds/package.go` — a package resource now attempts every package and aggregates failures, rather than stopping at the first one).
But the fix only addresses failures within a single resource. The deeper question is what `engine.DiffAndExecute` (`engine/resource.go`) does when a resource's `Execute` returns an error at all:
```go
if err := op.Execute(); err != nil {
return nil, fmt.Errorf("resource %s: %w", r.ID, err)
}
```
It returns immediately — the entire `apply` run stops, including every resource that comes after the failed one in dependency order, even resources with no relationship to the one that failed.
The gap
`pkg` (the base-package install resource) has no resources declaring `DependsOn: []string{"base_packages"}` on it anywhere — even though `engine/modules/registry.go`'s own comment says "pkg first, since other modules assume tools are already installed." That assumption is real but currently undeclared in the actual dependency graph; it only "works" because `pkg` happens to be registered first and a full-stop failure model doesn't need the graph to know about it.
If `pkg` (or any other resource) fails, right now nothing else converges at all — not because of a real dependency, but because the engine has no concept of "skip only what actually depends on the failure, keep converging what doesn't."
What to decide
- Should `DiffAndExecute` change to: on a resource failure, mark it and everything transitively depending on it (via `DependsOn`) as skipped, but continue executing every resource NOT in that dependency chain?
- If so, does `pkg` need explicit `DependsOn` edges declared FROM the resources that assume packages are installed (e.g. does `git.go`'s `GitConfig` need `DependsOn: []string{"base_packages"}`, does `services.go`'s atuin resource, etc.) — auditing every module for undeclared implicit ordering assumptions is real work, not a quick fix.
- How should partial-failure be reported to the user — a summary of what converged vs. what didn't, distinctly from today's single top-level error?
This is real architectural work, not a same-day fix — filed separately from the `installAll` fix so it doesn't block merging PR #10.
Context
Found during a real end-to-end Linux container test before merging PR #10: `lazygit` isn't packaged in Ubuntu's default apt repos, which caused `Package`'s install step to fail. That specific problem is fixed (see the `installAll` change in `engine/kinds/package.go` — a package resource now attempts every package and aggregates failures, rather than stopping at the first one).
But the fix only addresses failures within a single resource. The deeper question is what `engine.DiffAndExecute` (`engine/resource.go`) does when a resource's `Execute` returns an error at all:
```go
if err := op.Execute(); err != nil {
return nil, fmt.Errorf("resource %s: %w", r.ID, err)
}
```
It returns immediately — the entire `apply` run stops, including every resource that comes after the failed one in dependency order, even resources with no relationship to the one that failed.
The gap
`pkg` (the base-package install resource) has no resources declaring `DependsOn: []string{"base_packages"}` on it anywhere — even though `engine/modules/registry.go`'s own comment says "pkg first, since other modules assume tools are already installed." That assumption is real but currently undeclared in the actual dependency graph; it only "works" because `pkg` happens to be registered first and a full-stop failure model doesn't need the graph to know about it.
If `pkg` (or any other resource) fails, right now nothing else converges at all — not because of a real dependency, but because the engine has no concept of "skip only what actually depends on the failure, keep converging what doesn't."
What to decide
This is real architectural work, not a same-day fix — filed separately from the `installAll` fix so it doesn't block merging PR #10.