Skip to content

chore(shepherd): enable SHEPHERD_FIX_ONLY_SERVICES so the #292 path takes effect - #1202

Merged
mashkovd merged 1 commit into
mainfrom
chore/agents-292-enable-shepherd-fix-only-services
Sep 12, 2026
Merged

mashkovd merged 1 commit into
mainfrom
chore/agents-292-enable-shepherd-fix-only-services

Conversation

@mashkovd

Copy link
Copy Markdown
Member

The gap

mctlhq/mctl-agents#292 split shepherd ownership by stage rather than by repo:

  • fix-only — the shepherd discovers, reviews and pushes follow-up commits, but decide() returns defer-merge instead of merge, so the merge decision stays with the other PR lifecycle (pr-steward).
  • skip — discovered by nothing.

That is what gives a steward-owned repository a path from a blocking review finding back to code. Before it, the pr-steward could say what was wrong and had nothing that could fix it.

The code shipped in mctlhq/mctl-agents#341 on 2026-09-11 and has been dark ever since, because this value was never set. SHEPHERD_FIX_ONLY_SERVICES is read by run_shepherd.py, documented in its module docstring, in README.md and in ADR-006's #292 addendum — and set nowhere in this repo. Every steward-owned repo stayed in SHEPHERD_SKIP_SERVICES, so deployed behaviour was still the binary FULL/SKIP it had been before #292, and the review-feedback path was unreachable in production.

mctlhq/mctl-gitops#1073 is the standing evidence: blocking findings from claude[bot] and agy since 2026-09-09, and no actor able to address them.

Found while reconstructing the ownership graph for mctlhq/.github#57, where this is pilot path 3.

The change

-  - name: SHEPHERD_SKIP_SERVICES
-    value: "mctl-design,mctl-telegram,mctl-gitops,mctl-pairdesk,mctl-academy"
+  - name: SHEPHERD_FIX_ONLY_SERVICES
+    value: "mctl-design,mctl-telegram,mctl-gitops,mctl-pairdesk"
+  - name: SHEPHERD_SKIP_SERVICES
+    value: "mctl-academy"

These are the values the approved #292 proposal specifies — task 9 of agents-state/mctl-agents/proposals/issue-292-fix-lifecycle-steward-owned-repos-have-n/tasks.md.

One deviation from that task, stated rather than made silently

The task says to set the values "in both cronworkflow-mctl-agents-shepherd.yaml and cwft-mctl-agents-shepherd.yaml".

The CronWorkflow has no env block at all — it is a parameters-only wrapper (service, slug, dry_run) around the CWFT, so there is no second place to set this:

$ grep -n "env:\|SHEPHERD_" cronworkflow-mctl-agents-shepherd.yaml
(none)

The CWFT is the only location, and it is also the one the proposal's own rationale names as load-bearing — "the CWFT env is what DevLoopWorkflow._shepherd_tick inherits". Setting it once covers both the cron entry point and the in-loop tick.

Why mctl-academy stays in SKIP

It is the reason the two lists are not redundant: academy is skipped by the shepherd and absent from the steward's repo list, so nothing auto-merges it at all. Its clean-room content policy makes human CODEOWNER approval the last check before a question publishes. It is additionally hardened in code — NEVER_MERGE_SERVICES caps it at fix-only in _service_mode, so no value here can ever resolve it to full.

Blast radius and reversal

Four repos move from "shepherd discovers nothing" to "shepherd discovers and may push a follow-up commit". Merge authority does not move: decide() returns defer-merge for every one of them, and the pr-steward's own merge_mode is unchanged (never for mctl-gitops, when-green for the others).

Reversible by reverting this commit. Fix-only wins over skip, so the two list edits are order-independent and there is no half-applied state.

Verification after merge

Wait ~3 min for ArgoCD to sync (Argo snapshots templates at submit time), then watch mctlhq/mctl-gitops#1073 get a shepherd tick and a follow-up commit, with the merge still deferred.

Refs mctlhq/mctl-agents#292, mctlhq/mctl-gitops#1073, mctlhq/.github#57

🤖 Generated with Claude Code

https://claude.ai/code/session_01FsqpZXEnvZR8aTpZ6vXRaz

…akes effect

mctl-agents#292 split shepherd ownership by stage: fix-only means the shepherd
discovers, reviews and pushes follow-up commits, while decide() returns
defer-merge so the merge decision stays with the other PR lifecycle. That is
what gives a steward-owned repository a path from a blocking review finding
back to code -- the pr-steward can say what is wrong but has nothing that
fixes it.

The code shipped in mctl-agents#341 on 2026-09-11 and has been dark since,
because this value was never set. Every steward-owned repo stayed in
SHEPHERD_SKIP_SERVICES, so the review-feedback path #292 added was unreachable
in production and deployed behaviour was still the binary FULL/SKIP it had been
before. #1073 is the standing evidence: blocking findings
from claude[bot] and agy since 2026-09-09, and no actor able to address them.

Values are the ones the approved #292 proposal specifies, task 9 of
agents-state/mctl-agents/proposals/issue-292-fix-lifecycle-steward-owned-repos-have-n/tasks.md.

Deviation from that task, stated rather than made silently: it says to set the
values "in both cronworkflow-mctl-agents-shepherd.yaml and
cwft-mctl-agents-shepherd.yaml". The CronWorkflow has no env block at all -- it
is a parameters-only wrapper (service, slug, dry_run) around the CWFT, so there
is no second place to set this. The CWFT is the only one, and it is also the
one the proposal's own rationale names as load-bearing, since it is what
DevLoopWorkflow._shepherd_tick inherits. Setting it once covers both entry
points.

mctl-academy stays in SKIP and is why the two lists are not redundant: it is
skipped by the shepherd and absent from the steward's repo list, so nothing
auto-merges it at all. Its clean-room content policy makes human CODEOWNER
approval the last check before a question publishes.

Reversible by reverting this commit; fix-only wins over skip, so the two list
edits are order-independent.

Refs: mctlhq/mctl-agents#292, #1073, mctlhq/.github#57

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FsqpZXEnvZR8aTpZ6vXRaz
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@claude

claude Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 1m 15s —— View job


Review complete ✅

  • Gather context (diff, CLAUDE.md, related files)
  • Analyze correctness of the shepherd env var change
  • Check Argo Workflows conventions (CWFT env block, CronWorkflow)
  • Post findings / verdict

Findings: None (P1/P2/P3).

This is a values-only change to cwft-mctl-agents-shepherd.yaml: it splits the existing SHEPHERD_SKIP_SERVICES list into a new SHEPHERD_FIX_ONLY_SERVICES list plus a smaller SHEPHERD_SKIP_SERVICES, with comments explaining the new stage-based ownership model. No template logic, RBAC, resource limits, or secrets wiring changed.

Verified independently:

  • cronworkflow-mctl-agents-shepherd.yaml genuinely has no env: block (grep -n "env:\|SHEPHERD_\|kind:" only matches kind: CronWorkflow), confirming the PR's claim that the CWFT is the only place this can be set.
  • The two lists are disjoint and their union covers the same four services as before, plus mctl-academy unchanged in skip — no service silently dropped or duplicated.
  • YAML structure around the edited block is well-formed and consistent with the surrounding container spec.

Approving as a trivial config/values change per review policy.

gh api repos/mctlhq/mctl-gitops/pulls/1202/reviews --method POST -f commit_id="eea72099838c68a182d7718478e43096e5658b34" -f event="APPROVE" -f body="Trivial env-var/values change (splits SHEPHERD_SKIP_SERVICES into SHEPHERD_FIX_ONLY_SERVICES + SHEPHERD_SKIP_SERVICES); no logic/RBAC/secret changes. Verified CronWorkflow has no env block, confirming CWFT is the only required edit location."

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial env-var/values change (splits SHEPHERD_SKIP_SERVICES into SHEPHERD_FIX_ONLY_SERVICES + SHEPHERD_SKIP_SERVICES); no logic/RBAC/secret changes. Verified CronWorkflow has no env block, confirming CWFT is the only required edit location.

@github-actions

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high · commit eea7209.

Severity: P1
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 686
Anchor: + value: "mctl-design,mctl-telegram,mctl-gitops,mctl-pairdesk"
Failure Scenario: The repository mctl-gitops is the GitOps source of truth where any merged change directly modifies the cluster. Moving mctl-gitops from SHEPHERD_SKIP_SERVICES to SHEPHERD_FIX_ONLY_SERVICES grants the shepherd agent the ability to autonomously push follow-up fix commits to PRs. Because these PRs are steward-owned and automatically merged by the pr-steward agent once reviews pass, this change creates a fully automated pipeline where an AI agent can author, push, and (effectively) merge configuration changes directly into the cluster without any human CODEOWNER gate. A hallucinated or logically flawed YAML patch pushed by the agent would reach the cluster, leading to severe outages or security vulnerabilities.
Proposed Fix: Remove mctl-gitops from the SHEPHERD_FIX_ONLY_SERVICES list and restore it to SHEPHERD_SKIP_SERVICES to prevent agents from auto-merging generated code into the cluster's GitOps source of truth. Alternatively, implement a strict code-level human CODEOWNER approval requirement for mctl-gitops (similar to the existing NEVER_MERGE_SERVICES used for mctl-academy).

Severity: P3
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 686
Anchor: + value: "mctl-design,mctl-telegram,mctl-gitops,mctl-pairdesk"
Failure Scenario: The diff significantly modifies the target repository scopes by introducing new fix-only capabilities for several repositories that were previously skipped. There is no test visible in this diff to verify that the agent correctly parses these new lists, handles the fix-only pipeline logic, and correctly defers merges as intended without unintended side effects.
Proposed Fix: Add test coverage under tests/ to validate the new fix-only behavior and ensure the merge deferral logic works safely for these repositories.

@mashkovd

Copy link
Copy Markdown
Member Author

Response to the agy P1 — verified link by link, and the hardening is shipping

P1 — "an AI agent can author, push and effectively merge configuration changes directly into the cluster without any human CODEOWNER gate."

Checked rather than assumed. The chain does not close today, and the middle link is factually wrong for this repository:

Link Claim Reality
shepherd merges — mctl-gitops resolves to FIX_ONLY, so decide() returns defer-merge; merge_pr() refuses independently at run_shepherd.py:1626 and :1885
pr-steward merges "automatically merged by the pr-steward agent once reviews pass" False for this repo. pr-steward.config.json in services/labs/claude-remote/values.yaml:703-709 sets "name": "mctlhq/mctl-gitops" → "merge_mode": "never" — inventory and escalation only. The global default is never too.
auto-merge.yml merges — It fires only on startsWith(github.event.pull_request.head.ref, 'claude/'). Agent PRs are feat/agents-<slug> (run_implementer.py:441), so it never matches.

So the P1 is not confirmed as a live defect, and this PR does not create the pipeline it describes.

The proposed alternative fix is right, though, and it is shipping. All three of those guards are configuration — two of them in another repository — and one edit reopens the path. mctl-gitops is the GitOps source of truth: a merge here is not a change awaiting a release, it is a live cluster change, and the blast radius is the whole platform. Relying on config for that is the same config-versus-code conflation this repo's own ADR-006 was corrected for earlier today.

mctlhq/mctl-agents#357 adds mctl-gitops to NEVER_MERGE_SERVICES, which caps it at fix-only in _service_mode regardless of any env value and refuses independently in merge_pr. Both guards are proved by mutation — reverting the constant fails the new tests, restoring it passes them.

That PR lands before this one, so the code-level guarantee exists before the env var widens the shepherd's reach.

P3 — no test coverage for the new lists.

The behaviour these values select is tested in mctl-agents, not here: tests/test_run_shepherd.py covers _service_mode resolution for both env lists, the both-lists-overlap case, fix-only discovery, and decide() returning defer-merge — added with the #292 implementation in mctl-agents#341, and extended by #357 with a case that pins mctl-gitops specifically. A values-only change here selects tested behaviour rather than introducing untested behaviour.

What is genuinely missing is a check on the invariant between the two repositories — that the shepherd's lists and the steward's repo list stay a total, disjoint partition. Today that invariant is asserted only in a comment in claude-remote/values.yaml ("steward owns ONLY what the shepherd skips") and nothing enforces it. That is tracked as part of mctlhq/.github#57, where the two lists collapse into one lifecycle-policy.yaml read by both actors with CI asserting the partition — which is the right place to fix it, rather than adding a check here that would encode the split it is meant to remove.

@mashkovd
mashkovd merged commit e6206f9 into main Sep 12, 2026
20 checks passed
@mashkovd
mashkovd deleted the chore/agents-292-enable-shepherd-fix-only-services branch September 12, 2026 00:56
@mashkovd

Copy link
Copy Markdown
Member Author

Correction to this PR's verification section

Merged, and the rollout is live. But the verification step I wrote — "watch mctlhq/mctl-gitops#1073 get a shepherd tick and a follow-up commit" — is wrong, and I would rather correct it here than have someone wait for a tick that is never coming.

#1073 is proposal-less. Its head branch is feat/governed-auto-merge-gate, not feat/agents-<slug>, and there is no agents-state/mctl-gitops/proposals/ entry for it. The shepherd discovers work by globbing proposal .status.yaml files and then locating the PR; a PR with no proposal is invisible to it regardless of whether its service is skip, fix-only or full.

So #1073 needs both halves: this change (so the shepherd touches mctl-gitops at all) and mctlhq/mctl-agents#334 (proposal-less PR adoption, unshipped). This change alone does not move it. mctlhq/.github#38's own note said as much and I read it as "blocked on #292" when it is "blocked on #292 and #334".

What this change actually does, checked against current state

The shepherd now discovers proposals in mctl-design, mctl-telegram, mctl-gitops and mctl-pairdesk instead of skipping them. Actionable proposals — those in implemented / review-fixing / in-progress — right now:

service actionable
mctl-design 0
mctl-gitops 0 (28 accepted, which is the implementer's queue, not the shepherd's)
mctl-pairdesk 0
mctl-telegram 1 — issue-620-feat-clients-identity-attributes-and-pro → mctlhq/mctl-telegram#627

So there is exactly one PR this reaches today, and the honest prediction for it is wait, not a fix commit: #627 is BEHIND, which is not in MERGEABLE_STATES, and its only current-head blocking finding is a P2 from agy — which decide() does not parse, because GATING_BOTS is claude[bot] and chatgpt-codex-connector[bot] only. Claude has approved that head.

That is a fair demonstration of why mctlhq/mctl-agents#240 is on the epic: the ownership half is now correct and the input half still is not, so the newly-reachable owner looks at a PR with a real blocker and sees nothing to do.

Nothing here is unsafe — wait is a no-op — and the change stands on its own: it is the difference between a steward-owned repo having no path from findings back to code and having one. It just does not have a visible effect this minute, and claiming otherwise would have been the kind of unverified completion evidence this epic exists to stop producing.

Tracking the accurate verification target on mctlhq/.github#57.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant