From 975e7a97e47e1087a538534e9daedba70c05a9ec Mon Sep 17 00:00:00 2001 From: Michael Pursifull Date: Sun, 19 Apr 2026 01:35:54 -0500 Subject: [PATCH] fix(api): manifest parser drops max_restarts + dangerous_permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fields exist on api.Role with toml tags, the team controller honors MaxRestarts (the crash-loop saturation cap from PR #21), and the forestage adapter honors DangerousPermissions (PR #20). But ManifestRole in internal/api/manifest.go never declared either field, so yaml.v3 and BurntSushi/toml both silently dropped them during parse, and Apply() never copied them onto the constructed Role. Net effect: - ArcavenAE/marvel#28 — `max_restarts: 3` in a YAML manifest produced `Role.MaxRestarts = 0`, which means "unlimited" per the contract in types.go. The cap was permanently disabled on manifest-driven teams. - ArcavenAE/marvel#43 — `dangerous_permissions: true` produced `Role.DangerousPermissions = false`, so the forestage adapter never appended `--dangerously-skip-permissions`. Autonomous fleet-agent teams couldn't ship the flag from a manifest. Adds both fields to ManifestRole with matching `toml` + `yaml` struct tags, wires Apply() to copy them into Role, and adds regression tests covering the round-trip in both manifest formats plus a defaults test that pins the zero-valued behavior. Refs ArcavenAE/marvel#28 Refs ArcavenAE/marvel#43 --- internal/api/manifest.go | 34 ++++---- internal/api/manifest_test.go | 141 ++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 15 deletions(-) diff --git a/internal/api/manifest.go b/internal/api/manifest.go index eed14b7..56bff02 100644 --- a/internal/api/manifest.go +++ b/internal/api/manifest.go @@ -35,14 +35,16 @@ type ManifestTeam struct { // ManifestRole is a role section within a team. // Name is the job function. Persona and Identity are the costume and lens. type ManifestRole struct { - Name string `toml:"name" yaml:"name"` - Replicas int `toml:"replicas" yaml:"replicas"` - Runtime ManifestRuntime `toml:"runtime" yaml:"runtime"` - RestartPolicy string `toml:"restart_policy,omitempty" yaml:"restart_policy,omitempty"` - Permissions string `toml:"permissions,omitempty" yaml:"permissions,omitempty"` - Persona string `toml:"persona,omitempty" yaml:"persona,omitempty"` - Identity string `toml:"identity,omitempty" yaml:"identity,omitempty"` - HealthCheck *ManifestHealthCheck `toml:"healthcheck,omitempty" yaml:"healthcheck,omitempty"` + Name string `toml:"name" yaml:"name"` + Replicas int `toml:"replicas" yaml:"replicas"` + Runtime ManifestRuntime `toml:"runtime" yaml:"runtime"` + RestartPolicy string `toml:"restart_policy,omitempty" yaml:"restart_policy,omitempty"` + MaxRestarts int `toml:"max_restarts,omitempty" yaml:"max_restarts,omitempty"` + Permissions string `toml:"permissions,omitempty" yaml:"permissions,omitempty"` + DangerousPermissions bool `toml:"dangerous_permissions,omitempty" yaml:"dangerous_permissions,omitempty"` + Persona string `toml:"persona,omitempty" yaml:"persona,omitempty"` + Identity string `toml:"identity,omitempty" yaml:"identity,omitempty"` + HealthCheck *ManifestHealthCheck `toml:"healthcheck,omitempty" yaml:"healthcheck,omitempty"` } // ManifestHealthCheck is the healthcheck section within a role. @@ -227,13 +229,15 @@ func (m *Manifest) Apply(store *Store) error { rt.Name = rt.Command } role := Role{ - Name: mr.Name, - Replicas: mr.Replicas, - Runtime: rt, - RestartPolicy: RestartAlways, - Permissions: mr.Permissions, - Persona: mr.Persona, - Identity: mr.Identity, + Name: mr.Name, + Replicas: mr.Replicas, + Runtime: rt, + RestartPolicy: RestartAlways, + MaxRestarts: mr.MaxRestarts, + Permissions: mr.Permissions, + DangerousPermissions: mr.DangerousPermissions, + Persona: mr.Persona, + Identity: mr.Identity, } if mr.RestartPolicy != "" { role.RestartPolicy = RestartPolicy(mr.RestartPolicy) diff --git a/internal/api/manifest_test.go b/internal/api/manifest_test.go index fb116b2..be776cc 100644 --- a/internal/api/manifest_test.go +++ b/internal/api/manifest_test.go @@ -414,6 +414,147 @@ func TestManifestApply(t *testing.T) { } } +// TestParseYAMLManifestDroppedFields is the regression test for +// ArcavenAE/marvel#28 (max_restarts) and #43 (dangerous_permissions). +// Both fields existed on api.Role and were honored by the team +// controller and forestage adapter respectively — but ManifestRole +// didn't declare them, so yaml.v3 silently dropped them during parse, +// and Apply() never copied them onto Role. Effect: the cap was +// permanently disabled and --dangerously-skip-permissions never made +// it to the adapter. +func TestParseYAMLManifestDroppedFields(t *testing.T) { + t.Parallel() + m, err := parseManifestYAML([]byte(` +workspace: + name: test + +teams: + - name: squad + roles: + - name: worker + replicas: 1 + max_restarts: 3 + dangerous_permissions: true + runtime: + image: forestage + command: forestage +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + role := m.Teams[0].Roles[0] + if role.MaxRestarts != 3 { + t.Errorf("MaxRestarts on ManifestRole: got %d, want 3", role.MaxRestarts) + } + if !role.DangerousPermissions { + t.Errorf("DangerousPermissions on ManifestRole: got false, want true") + } + + // Full round-trip to api.Role via Apply must carry both fields. + store := NewStore() + if err := m.Apply(store); err != nil { + t.Fatalf("apply: %v", err) + } + team, err := store.GetTeam("test/squad") + if err != nil { + t.Fatalf("get team: %v", err) + } + if team.Roles[0].MaxRestarts != 3 { + t.Errorf("MaxRestarts on api.Role after Apply: got %d, want 3", team.Roles[0].MaxRestarts) + } + if !team.Roles[0].DangerousPermissions { + t.Errorf("DangerousPermissions on api.Role after Apply: got false, want true") + } +} + +// TestParseTOMLManifestDroppedFields is the TOML-side twin of +// TestParseYAMLManifestDroppedFields. TOML was already honoring the +// toml struct tags on api.Role directly (Role is used in some code +// paths without going through ManifestRole), but the manifest parse +// path is the same — ManifestRole was missing the fields, so TOML +// manifests silently dropped them too. +func TestParseTOMLManifestDroppedFields(t *testing.T) { + t.Parallel() + m, err := parseManifestTOML([]byte(` +[workspace] +name = "test" + +[[team]] +name = "squad" + + [[team.role]] + name = "worker" + replicas = 1 + max_restarts = 3 + dangerous_permissions = true + + [team.role.runtime] + image = "forestage" + command = "forestage" +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + role := m.Teams[0].Roles[0] + if role.MaxRestarts != 3 { + t.Errorf("MaxRestarts on ManifestRole: got %d, want 3", role.MaxRestarts) + } + if !role.DangerousPermissions { + t.Errorf("DangerousPermissions on ManifestRole: got false, want true") + } + + store := NewStore() + if err := m.Apply(store); err != nil { + t.Fatalf("apply: %v", err) + } + team, err := store.GetTeam("test/squad") + if err != nil { + t.Fatalf("get team: %v", err) + } + if team.Roles[0].MaxRestarts != 3 { + t.Errorf("MaxRestarts on api.Role after Apply: got %d, want 3", team.Roles[0].MaxRestarts) + } + if !team.Roles[0].DangerousPermissions { + t.Errorf("DangerousPermissions on api.Role after Apply: got false, want true") + } +} + +// TestParseManifestDroppedFieldsDefaults verifies that omitting both +// fields produces zero values (MaxRestarts=0 meaning unlimited, +// DangerousPermissions=false meaning the adapter does not append the +// flag). Guards against accidental non-zero defaults that would break +// the documented contract. +func TestParseManifestDroppedFieldsDefaults(t *testing.T) { + t.Parallel() + m, err := parseManifestYAML([]byte(` +workspace: + name: test + +teams: + - name: squad + roles: + - name: worker + replicas: 1 + runtime: + command: sleep + args: ["300"] +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + store := NewStore() + if err := m.Apply(store); err != nil { + t.Fatalf("apply: %v", err) + } + team, _ := store.GetTeam("test/squad") + if team.Roles[0].MaxRestarts != 0 { + t.Errorf("MaxRestarts default: got %d, want 0", team.Roles[0].MaxRestarts) + } + if team.Roles[0].DangerousPermissions { + t.Errorf("DangerousPermissions default: got true, want false") + } +} + func TestValidateRuntimesOK(t *testing.T) { t.Parallel() // Any two binaries guaranteed on POSIX test hosts.