feat(agents): issue-161-ci-roadmap-fail-validation-when-a-change by mctl-agents[bot] · Pull Request #193 · mctlhq/.github

mctl-agents · 2026-10-16T00:10:42Z

Implements accepted proposal .github/issue-161-ci-roadmap-fail-validation-when-a-change.

Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/.github/proposals/issue-161-ci-roadmap-fail-validation-when-a-change/

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

Closes #161

Add a diff-aware lifecycle guard that fails when a completed epic gains required work, wire it into roadmap-validate.yml with full history, and document it in the roadmap README. Proposal: platform-gitops/agents-state/.github/proposals/issue-161-ci-roadmap-fail-validation-when-a-change/ Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

claude · 2026-10-16T00:11:11Z

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

Review of commit `71e7d4a`: changes requested

I've requested changes because of one P2 finding. There are no P1 findings and two P3 nits. I only reviewed the code by reading it; I couldn't run the tests here because the command needed approval I don't have.

Read the pinned diff (33af5ff...71e7d4a)
Reviewed the workflow, lifecycle_guard.py, the new test helpers and the tests
Posted inline comments (pinned to commit 71e7d4a)
Submitted the verdict (pinned to commit 71e7d4a)

P2: the pull-request check compares against the wrong base (.github/workflows/roadmap-validate.yml:41)

On a pull request, the checkout is the merge of base.sha and the PR head. That means the head files already include everything merged to main since the branch was cut.
The step compares those files against git merge-base. Anything that landed on main since the branch point therefore looks like part of this PR.
Example: an epic is added on main as active and later marked completed. Every older open PR that touches roadmap/** then fails with BornCompletedUnverified, even though it never touched that epic.
Fix: pass --base-ref "$BASE_SHA" directly, and update the README sentence that says the guard compares with the merge base. Fix this →

P3 nits (not blocking)

roadmap/tests/test_lifecycle_guard.py:86: test_load_bearing always passes. It replaces check with a stub and then asserts that the stub's result is wrong, so it tests nothing about the real check.
roadmap/scripts/lifecycle_guard.py:108: the base side lists manifests recursively, but the head side only looks at the top level of roadmap/epics and keeps just the file name. A YAML file in a subfolder would be read on one side and ignored on the other. This can't happen with today's flat folder.

Checked and fine:

${{ }} values reach the scripts only through env:, so nothing can be injected into the run: commands.
permissions: {} is kept, and the guard makes no network calls.
git is always called with a fixed argument list.
A missing or broken base exits with code 2 instead of passing.
A push with an all-zero before produces a visible warning rather than a silent pass.

claude · 2026-10-16T00:12:44Z

+          BASE_SHA: ${{ github.event.pull_request.base.sha }}
+          HEAD_SHA: ${{ github.event.pull_request.head.sha }}
+        run: |
+          base="$(git merge-base "$BASE_SHA" "$HEAD_SHA")"


P2 — compares against the merge-base, but the checked-out tree is the merge commit.

On pull_request, actions/checkout checks out refs/pull/N/merge, which is base.sha merged with the PR head. So the head manifests (--head-dir roadmap/epics) already include everything that landed on main after the branch point. Using git merge-base "$BASE_SHA" "$HEAD_SHA" as the base blames this PR for those main-side changes.

Concrete false positive: after a branch is cut, an epic is created on main as active and later moved to completed (both legitimate). For any older open PR touching roadmap/**, that epic is missing from the merge-base and completed in the merge tree, so the guard reports BornCompletedUnverified and fails the unrelated PR. The same thing happens for an epic that, on main, was reopened, given a new required item and completed again: the PR gets RequiredItemAdded.

The first parent of the checked-out merge commit is BASE_SHA, so compare against it directly:

run: python3 roadmap/scripts/lifecycle_guard.py --base-ref "$BASE_SHA"

HEAD_SHA is then unused. The README sentence "compares the head manifests with the merge base" should be updated too.

claude · 2026-10-16T00:12:49Z

+        found = guard.check([state(FINOPS)], [state(head, "roadmap/epics/moved.yaml")])
+        self.assertEqual(codes(found), ["RequiredItemAdded"])
+
+    def test_load_bearing(self):


P3 — tautological test. This patches guard.check to return [] and then asserts that the assertion fails, so it passes no matter what the real check does. It does not show that the guard is load-bearing. Either delete it, or turn it into a real control, e.g. a CLI-level test that the mutated corpus exits 1 with the real check and exits 0 when check is stubbed out.

claude · 2026-10-16T00:12:53Z

+        raise BaseError(f"base-unresolvable: {ref!r} is not a commit here{hint}")
+    commit = resolved.stdout.decode().strip()
+
+    listing = _git(repo, "ls-tree", "-rz", "--name-only", commit, "--", subdir)


P3 — base and head enumerate manifests differently. The base uses ls-tree -r, which is recursive, while the head uses validate._manifest_paths, which only globs roadmap/epics/*.yaml|*.yml at the top level, and then rebuilds the path as roadmap/epics/<basename> (L176).

If a YAML file is ever added under a subdirectory of roadmap/epics/, the base side will parse it and may exit 2 with base-malformed, while the head side silently ignores it. Path-fallback pairing would also never match it. Not reachable with the current flat layout, but enumerating the same way on both sides would remove the asymmetry. Note that test_requires_recursive_listing currently locks in the recursive behaviour.

claude

Has P1/P2 findings, changes requested (0 P1, 1 P2, 2 P3): the PR guard compares the merge-commit checkout against the merge-base instead of base.sha, so changes that landed on main after the branch point show up as false RequiredItemAdded/BornCompletedUnverified failures on unrelated PRs.

github-actions · 2026-10-16T00:18:15Z

Antigravity (agy) review — informational

Model: gemini-3.8-flash-high · commit 71e7d4a.

No significant issues found.

…l-validation-when-a-change Compare against the PR base sha (first parent of the checked-out merge commit) instead of the merge-base, so main-side changes are not blamed on the PR. README wording updated. Finding 2 is the review summary of finding 1 and is resolved by the same change. Proposal: platform-gitops/agents-state/.github/proposals/issue-161-ci-roadmap-fail-validation-when-a-change/ Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

mctl-agents · 2026-10-16T00:28:13Z

@claude review

claude · 2026-10-16T00:28:34Z

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

Review of commit `bf31c84`: approved

I approved this commit. It has no P1 or P2 findings; the two P3 nits from the last review are still open. I only reviewed the code by reading it. I couldn't run the tests here because the command needed approval I don't have.

Read the fix commit diff (71e7d4a...bf31c84) and the full pinned diff
Re-checked the workflow and the guard
Run the tests (blocked: the command needed approval)
Submitted the verdict (APPROVE, pinned to bf31c84)

The P2 from the last review is fixed (.github/workflows/roadmap-validate.yml:40)

The pull-request step now runs lifecycle_guard.py --base-ref "$BASE_SHA" directly.
The checked-out merge commit's first parent is base.sha. Changes that landed on main after the branch point now appear on both sides, so they no longer show up as false BornCompletedUnverified or RequiredItemAdded failures on unrelated PRs.
HEAD_SHA was removed, and the README now describes the comparison correctly.

Still open (P3, not blocking)

roadmap/tests/test_lifecycle_guard.py:86: test_load_bearing still replaces check with a stub and then tests the stub, so it always passes.
roadmap/scripts/lifecycle_guard.py:108: the base side lists manifests recursively (ls-tree -r), while the head side reads only the top level of roadmap/epics. This can't cause a problem with today's flat folder.

Checked again and fine:

permissions: {} is kept.
${{ }} values still reach the scripts only through env:.
git is always called with a fixed argument list.
A missing or unreadable base still exits with code 2 instead of passing.
The push path, including the warning for an all-zero before, has not changed.

claude

No P1/P2 findings (2 P3, carried over from the previous review and still open: tautological test_load_bearing; base and head list manifests differently). The P2 merge-base false positive is fixed: the guard now compares against base.sha, which is the merge commit's first parent. Good to merge.

github-actions · 2026-10-16T00:36:44Z

Antigravity (agy) review

agy did not produce a review on this PR; see the workflow log. Not blocking merge — no verdict was reached, so there is nothing to act on.

claude Bot reviewed Oct 16, 2026

View reviewed changes

claude Bot requested changes Oct 16, 2026

View reviewed changes

claude Bot approved these changes Oct 16, 2026

View reviewed changes

Navigation Menu

Repository navigation

feat(agents): issue-161-ci-roadmap-fail-validation-when-a-change - #193

feat(agents): issue-161-ci-roadmap-fail-validation-when-a-change#193
mctl-agents[bot] wants to merge 2 commits into
mainfrom
feat/agents-issue-161-ci-roadmap-fail-validation-when-a-change

mctl-agents Bot commented Oct 16, 2026

Uh oh!

claude Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

claude Bot Oct 16, 2026

Uh oh!

claude Bot Oct 16, 2026

Uh oh!

claude Bot Oct 16, 2026

Uh oh!

claude Bot left a comment

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

mctl-agents Bot commented Oct 16, 2026

Uh oh!

claude Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

claude Bot left a comment

Uh oh!

github-actions Bot commented Oct 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

Conversation

mctl-agents Bot commented Oct 16, 2026

Uh oh!

claude Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of commit 71e7d4a: changes requested

Uh oh!

claude Bot Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Oct 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 16, 2026

Antigravity (agy) review — informational

Uh oh!

mctl-agents Bot commented Oct 16, 2026

Uh oh!

claude Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of commit bf31c84: approved

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 16, 2026

Antigravity (agy) review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

claude Bot commented Oct 16, 2026 •

edited

Loading

Review of commit `71e7d4a`: changes requested

claude Bot commented Oct 16, 2026 •

edited

Loading

Review of commit `bf31c84`: approved