[mctl-agents]

Implements accepted proposal mctl-gitops/issue-950-feat-agent-platform-add-gitops-agentdefi.

Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-gitops/proposals/issue-950-feat-agent-platform-add-gitops-agentdefi/

Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.

Closes #950

[claude]

Claude finished @mctl-agents[bot]'s task in 5m 15s —— View job

---

Reviewing PR #953

• Gather context (diff, CLAUDE.md, changed files)
• Understand request (automated code review per custom instructions)
• Review correctness / operational safety of agent-platform manifests, schemas, policy
• Review validate-agent-platform.py script and CI wiring
• Review test fixtures for coverage gaps
• Post inline findings and submit verdict

Note on diff resolution: this checkout is shallow, and the given BASE SHA (09da7b57...) is not an ancestor of COMMIT SHA — both are siblings branched from 803636c3 (a sibling PR, #952-related "issue-227" commit, merged in parallel). git diff BASE...HEAD therefore has no merge-base and errors. Since both SHAs share 803636c3 as their sole direct parent, I diffed 803636c3..HEAD instead, which is equivalent to the intended three-dot diff and matches the PR's own reported stats exactly (36 files, +1963/-0).

Summary

This is a clean, additive-only PR implementing the GitOps half of ADR 007: an agent-platform/ catalog (policy.yaml ceilings/reference catalogs, three ExecutionProfiles, three ReleaseBindingIntent shadow fixtures, two JSON Schemas), scripts/validate-agent-platform.py, and its CI wiring in validate-manifests.yml. Reviewed the full diff, cross-checked every profile's tools/skills/policyRef/mutationScopes/approval against policy.yaml, and read all 15 invalid + 2 valid selftest fixtures against the validator logic — the fixtures accurately exercise what they claim (unknown-tool/skill/model-task, missing required fields, budget/timeout ceiling, mutation-without-approval, unapproved sandbox, no global lifecycleState, ambiguous/incompatible/disabled profile resolution, and exact-pair rollback vs. independent-half rollback). The three default profiles correctly preserve today's effective production budget/timeout values per the README's extraction table, and CI ordering/exclusions (kubeconform doesn't touch agent-platform/, since these are custom-schema resources, not k8s CRDs) look right.

Posted 2 inline findings:

• P2 (scripts/validate-agent-platform.py:198,202 / policy.yaml:117-118 in the script): Policy.__init__ reads limits.maxBudgetUsd/maxTimeoutSeconds via plain .get() with no schema backing policy.yaml itself. A future typo or dropped key there silently sets the ceiling to None, and the ceiling checks (if policy.max_budget_usd is not None and ...) skip entirely — the one place this otherwise consistently fail-closed validator would fail open. Every other policy list already fails closed on the same kind of omission (empty set → everything "unknown").
• P3: NAME_RE in the script is defined but never used — dead code, since the JSON Schemas already enforce the same name pattern.

Verdict: Changes requested (1 P2, 1 P3) — submitted via gh api ... reviews bound to commit 0acb76ab.

[claude]

P2: policy.yaml has no JSON Schema, and Policy.__init__ reads limits.get("maxBudgetUsd")/limits.get("maxTimeoutSeconds") (lines 117-118) with no default-object validation. If a future edit to policy.yaml typos or drops spec.limits.maxBudgetUsd/maxTimeoutSeconds, policy.max_budget_usd/max_timeout_seconds silently becomes None, and the ceiling checks here (if policy.max_budget_usd is not None and ...) are skipped entirely rather than failing closed — the one place in this validator that fails open on missing config, contradicting the module docstring's "never a silent fallback" guarantee for "a budget or timeout above the policy ceiling." Every other policy list (knownTools, knownMutationScopes, approvedSandboxes, etc.) defaults to an empty collection on a missing/malformed key, which correctly fails closed (everything becomes "unknown"); only these two scalar ceilings degrade to unconstrained. Consider raising in Policy.__init__/load_policy when limits.maxBudgetUsd/maxTimeoutSeconds are absent or non-numeric, and add a selftest fixture for it alongside the existing missing-bounds/budget-exceeds-ceiling cases.

[claude]

P3: NAME_RE is defined but never referenced anywhere else in the file — metadata.name patterning is already enforced by the JSON Schema's own pattern (same regex, duplicated as a string literal in both schema files). Dead code; either wire it up as a redundant belt-and-braces check or drop it.

[claude]

Has P1/P2 findings, changes requested: 1 P2 (policy.yaml's budget/timeout ceilings fail open, not closed, if maxBudgetUsd/maxTimeoutSeconds are ever typo'd or dropped from policy.yaml — the one exception to this validator's otherwise-consistent fail-closed design), 1 P3 (dead NAME_RE regex).

[mctl-claude-remote]

@claude review

[claude]

Claude finished @mctl-claude-remote[bot]'s task in 1m 52s —— View job

---

Reviewing PR #953 (commit 77b66b0f)

• Gather context (diff, CLAUDE.md, changed files)
• Diff 0acb76ab..77b66b0f to see what changed since last review
• Verify P2/P3 findings from previous review were addressed
• Review new/changed lines for additional issues
• Post verdict

Summary

This commit (fix: address review findings) is a targeted follow-up to the earlier review of 0acb76ab. It touches exactly 2 files (+48/-8):

• scripts/validate-agent-platform.py: fixes the previously-flagged P2 — Policy.__init__ used to read limits.get("maxBudgetUsd")/maxTimeoutSeconds with no validation, so a missing/typo'd key silently became None and skipped the ceiling check (the one place the validator failed open). It now routes both through a new require_limit() helper that raises CatalogValidationError when the value is missing, non-numeric, or a bool (guards against Python's bool being an int subclass) — consistent with how every other policy list already fails closed. The now-unconditional ceiling checks in validate_profile_file are safe given this guarantee. load_policy()/case_policy_path() were also refactored to accept an explicit fixture path so selftest cases can ship their own policy.yaml.
• New selftest fixture scripts/tests/fixtures/agent-platform/invalid/policy-missing-ceiling/policy.yaml: declares only maxTimeoutSeconds, omitting maxBudgetUsd, to exercise the fix. Traced through run_selftest() — this correctly surfaces as a policy-load error via load_policy's existing try/except, so the case is picked up as an expected failure.

The P3 (dead NAME_RE regex) from the previous review was not addressed — it's still defined and unused. Non-blocking per the original triage.

I traced the fix logic statically (line-by-line review of require_limit, Policy.__init__, load_policy, case_policy_path, and run_selftest) rather than executing the script, since this environment doesn't permit running arbitrary shell/python commands — the sandbox declined python3 scripts/validate-agent-platform.py --selftest even with an explicit override. If you'd like actual execution confirmation (not just static trace), rerun CI's "Validate agent platform catalog" step or grant shell execution permissions.

Verdict: APPROVE — 0 P1/P2 findings, 1 pre-existing non-blocking P3. Submitted via gh api ... reviews bound to commit 77b66b0f.

[claude]

No P1/P2 findings (1 pre-existing P3, non-blocking). The prior P2 (policy.yaml budget/timeout ceilings failing open on a missing/non-numeric maxBudgetUsd or maxTimeoutSeconds) is fixed: Policy.init now routes both through a new require_limit() that raises CatalogValidationError when the value is missing, non-numeric, or a bool, restoring fail-closed behavior consistent with every other policy list. A new invalid/policy-missing-ceiling selftest fixture (only maxTimeoutSeconds declared) exercises the fix via load_policy's existing try/except, correctly surfacing as an invalid-fixture failure. The now-unconditional ceiling checks in validate_profile_file are safe given require_limit guarantees a numeric value. Scope of the fix commit is minimal (2 files, +48/-8) and touches nothing else. The unused NAME_RE regex (P3, dead code) from the previous review is still present but remains a non-blocking nit.