fix(api): manifest parser drops max_restarts + dangerous_permissions#47
Merged
fix(api): manifest parser drops max_restarts + dangerous_permissions#47
Conversation
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: - #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. - #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 #28 Refs #43
arcaven
added
type.bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes two latent bugs that share a single root cause:
ManifestRoleis missingmax_restartsanddangerous_permissions. Both fields exist onapi.Role, the team controller honorsMaxRestarts(PR #21), and the forestage adapter honorsDangerousPermissions(PR #20) — but the parsers drop the values andApply()never copies them, so they never reach runtime.max_restartssilently ignored) — cap was permanently disabled on manifest-driven teams.MaxRestarts=0means unlimited pertypes.go, so the reconciler never saturated.dangerous_permissions: truein a manifest producedRole.DangerousPermissions = false, so the forestage adapter never appended--dangerously-skip-permissions. Autonomous fleet-agent teams couldn't ship the flag from a manifest.Changes
internal/api/manifest.go— addMaxRestarts intandDangerousPermissions booltoManifestRolewith matchingtoml+yamltags. Copy both into the constructedRoleinApply().internal/api/manifest_test.go— three new tests:TestParseYAMLManifestDroppedFields— round-trip YAML with both fields set, verify on bothManifestRoleand the appliedapi.RoleTestParseTOMLManifestDroppedFields— same shape for TOMLTestParseManifestDroppedFieldsDefaults— omitting both fields produces zero values (pins the documented contract)Test plan
go test ./internal/api/... -run 'DroppedFields|DroppedFieldsDefaults'go test -race ./...golangci-lint run ./...— 0 issuesmax_restarts: 3saturates at 3 restarts; manifest withdangerous_permissions: trueproducesexec:line containing--dangerously-skip-permissionson a forestage roleWhy this slipped through
Both PR #20 and PR #21 added the fields to
api.Roleand the behavior code paths, along with unit tests that constructedapi.Roledirectly. Neither PR touchedManifestRoleorApply(). The manifest parse path had no coverage for these specific fields, so the yaml/toml silent-drop was invisible to CI.The
ManifestRole/api.Rolesplit deserves scrutiny in follow-up: a single struct with shared tags would make this class of drift impossible. Deliberately out of scope here — this PR is the point fix.Closes #28
Closes #43