From 2e9257fe1013f622aa7154405b826ac459a8f3fb Mon Sep 17 00:00:00 2001 From: Himakara Pieris Date: Thu, 10 Sep 2026 20:53:13 -0700 Subject: [PATCH] Fix: check_pr_ready.memory_updated no longer fails on .dmx/jobs/ writes (GH-37) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit memory_updated graded all of .dmx/ for uncommitted changes, so the loop runtime's own bookkeeping under .dmx/jobs/ (written by loop_advance at the human-gate pause, per GH-23) was mistaken for a forgotten memory-bank edit. A good release loop — PR opened, memory bank already committed by create-pr — failed memory_updated purely because its own paused job-state JSON was dirty, with no way to /loop-continue afterward since the loop was already complete. _dirty_dmx_files now excludes .dmx/jobs/ specifically, via git status -- '.dmx' ':!.dmx/jobs', instead of narrowing to a file extension. Everything else under .dmx/ (memory bank files, shared-sources.yaml, any other top-level file) is still checked exactly as before — this only removes the runtime's own state directory from the grade, nothing else. Fixes #37 --- CHANGELOG.md | 1 + src/dmx/validators/check_pr_ready.py | 26 +++++++--- tests/test_validators.py | 78 ++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ef6c96..002e473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Versioning follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed +- **(GH-37)** `check_pr_ready`'s `memory_updated` check now excludes `.dmx/jobs/` specifically, rather than grading all of `.dmx/`. Previously, the bundled `release` loop's own uncommitted job-state write (`.dmx/jobs/{job_id}/release-*.json`, written by `loop_advance` at the human-gate pause — see GH-23) was graded as a forgotten memory-bank edit, failing `memory_updated` on an otherwise-good PR that had already committed its `.dmx/*.md` changes. Everything else under `.dmx/` — memory bank files, `.dmx/shared-sources.yaml`, any other top-level file — is still checked exactly as before. - **(GH-36)** `find_active_run` no longer mistakes a skill's own JSON artifact (e.g. `validate` writing `.dmx/jobs/{job_id}/validation-report.json`) for a second, permanently-non-terminal loop run. A file now has to actually look like loop state — carry `loop_name`, `task_id`, and `status` — before it's considered a candidate at all; anything else in the job directory is skipped outright rather than defaulting to "non-terminal" when `status` is absent. Previously, a correct `validate` run left two JSON files in the same directory and `loop_advance`/`loop_continue` raised `AmbiguousActiveRun` on every attempt to proceed, with no fix short of hand-editing the artifact to fake a `status: complete` it doesn't have. ## [0.4.0] — 2026-09-09 diff --git a/src/dmx/validators/check_pr_ready.py b/src/dmx/validators/check_pr_ready.py index e971286..7cd7d09 100644 --- a/src/dmx/validators/check_pr_ready.py +++ b/src/dmx/validators/check_pr_ready.py @@ -8,10 +8,19 @@ repo for a real ticketing-API-backed check. ``memory_updated`` fails if ``.dmx/`` has *any* uncommitted changes (staged -or not) — a skill that edits the memory bank without committing (see GH-15) -would otherwise leave dangling working-tree state that's silently excluded -from the PR. Only once the tree is clean does it fall back to checking -whether the latest commit touched ``.dmx/*.md``. +or not), **excluding** ``.dmx/jobs/`` — a skill that edits the memory bank +(or any other tracked ``.dmx/`` file, e.g. ``shared-sources.yaml``) without +committing (see GH-15) would otherwise leave dangling working-tree state +that's silently excluded from the PR. Only once the tree is clean does it +fall back to checking whether the latest commit touched ``.dmx/*.md``. + +``.dmx/jobs/`` is excluded, not just narrowed to ``*.md``, because it's the +loop runtime's own bookkeeping (see GH-23), not a skill's uncommitted +edit — it's routinely dirty mid-loop (e.g. the paused state ``loop_advance`` +writes right after ``create-pr``, before ``_commit_dmx_state`` runs). +Grading it here produced a false failure on an otherwise-good PR (see +GH-37). Everything else under ``.dmx/`` — memory bank files, config, +``shared-sources.yaml``, any future top-level file — is still checked. Contract -------- @@ -90,10 +99,13 @@ def _ticket_transitioned(loop_context: dict[str, Any]) -> tuple[bool, str]: def _dirty_dmx_files(workspace_root: Path) -> list[str]: - """Return paths under .dmx/ with uncommitted changes (staged or not).""" + """Return paths under ``.dmx/`` with uncommitted changes, excluding + ``.dmx/jobs/`` (the loop runtime's own state — see the module docstring + and GH-37 for why it must not be graded here). + """ try: proc = subprocess.run( - ["git", "status", "--short", "--", ".dmx/"], + ["git", "status", "--short", "--", ".dmx", ":!.dmx/jobs"], cwd=workspace_root, capture_output=True, text=True, @@ -112,6 +124,8 @@ def _memory_updated(workspace_root: Path) -> tuple[bool, str]: if dirty: # A skill (e.g. update-memory) edited .dmx/ without committing — # this loop's memory sync isn't actually reflected in the PR. + # .dmx/jobs/ is excluded — that's the runtime's own bookkeeping, + # not a skill's uncommitted edit (see GH-37). return False, ( f"Uncommitted changes under .dmx/ ({', '.join(dirty)}) — " "memory bank edits must be committed, not left dangling in the " diff --git a/tests/test_validators.py b/tests/test_validators.py index 7f83069..5a65be4 100644 --- a/tests/test_validators.py +++ b/tests/test_validators.py @@ -512,6 +512,84 @@ def test_dirty_dmx_files_detects_real_uncommitted_changes(self, tmp_path: Path) memory_check = next(c for c in result["checks"] if c["name"] == "memory_updated") assert memory_check["pass"] is False + def test_dirty_job_state_does_not_fail_memory_check(self, tmp_path: Path) -> None: + """GH-37: an uncommitted .dmx/jobs/ write (the loop runtime's own + bookkeeping — see GH-23) is not a memory-bank edit and must not + fail memory_updated, even though it's genuinely dirty under .dmx/. + + Mirrors the real release-loop sequence: create-pr commits + .dmx/*.md, then loop_advance writes the paused job-state JSON + without committing it, then check_pr_ready runs against that tree. + """ + self._run_git(tmp_path, "init", "-q") + self._run_git(tmp_path, "config", "user.email", "test@example.com") + self._run_git(tmp_path, "config", "user.name", "Test") + dmx = tmp_path / ".dmx" + dmx.mkdir() + (dmx / "activeContext.md").write_text("committed by create-pr\n", encoding="utf-8") + self._run_git(tmp_path, "add", ".") + self._run_git(tmp_path, "commit", "-q", "-m", "chore: sync memory bank") + + # Simulate loop_advance's uncommitted pause-state write. + jobs_dir = dmx / "jobs" / "PAY-1" + jobs_dir.mkdir(parents=True) + (jobs_dir / "release-task1.json").write_text('{"status": "paused"}\n', encoding="utf-8") + + assert check_pr_ready._dirty_dmx_files(tmp_path) == [] + + result = check_pr_ready.run(tmp_path, {"ticket_ref": None}) + memory_check = next(c for c in result["checks"] if c["name"] == "memory_updated") + assert memory_check["pass"] is True + + def test_dirty_memory_bank_file_still_fails_alongside_dirty_job_state( + self, tmp_path: Path + ) -> None: + """A genuinely forgotten .dmx/*.md edit must still fail even when + .dmx/jobs/ is also dirty at the same time.""" + self._run_git(tmp_path, "init", "-q") + self._run_git(tmp_path, "config", "user.email", "test@example.com") + self._run_git(tmp_path, "config", "user.name", "Test") + dmx = tmp_path / ".dmx" + dmx.mkdir() + (dmx / "activeContext.md").write_text("initial\n", encoding="utf-8") + self._run_git(tmp_path, "add", ".") + self._run_git(tmp_path, "commit", "-q", "-m", "initial commit") + + (dmx / "activeContext.md").write_text("edited without committing\n", encoding="utf-8") + jobs_dir = dmx / "jobs" / "PAY-1" + jobs_dir.mkdir(parents=True) + (jobs_dir / "release-task1.json").write_text('{"status": "paused"}\n', encoding="utf-8") + + assert check_pr_ready._dirty_dmx_files(tmp_path) == [".dmx/activeContext.md"] + + result = check_pr_ready.run(tmp_path, {"ticket_ref": None}) + memory_check = next(c for c in result["checks"] if c["name"] == "memory_updated") + assert memory_check["pass"] is False + + def test_dirty_non_markdown_top_level_file_still_fails(self, tmp_path: Path) -> None: + """The fix for GH-37 excludes .dmx/jobs/ specifically — it must not + widen into "only .md files count". A dirty top-level non-.md file + (e.g. shared-sources.yaml) must still be caught, same as before.""" + self._run_git(tmp_path, "init", "-q") + self._run_git(tmp_path, "config", "user.email", "test@example.com") + self._run_git(tmp_path, "config", "user.name", "Test") + dmx = tmp_path / ".dmx" + dmx.mkdir() + (dmx / "activeContext.md").write_text("initial\n", encoding="utf-8") + (dmx / "shared-sources.yaml").write_text("shared_sources: []\n", encoding="utf-8") + self._run_git(tmp_path, "add", ".") + self._run_git(tmp_path, "commit", "-q", "-m", "initial commit") + + (dmx / "shared-sources.yaml").write_text( + "shared_sources: [{name: acme}]\n", encoding="utf-8" + ) + + assert check_pr_ready._dirty_dmx_files(tmp_path) == [".dmx/shared-sources.yaml"] + + result = check_pr_ready.run(tmp_path, {"ticket_ref": None}) + memory_check = next(c for c in result["checks"] if c["name"] == "memory_updated") + assert memory_check["pass"] is False + # --------------------------------------------------------------------------- # End-to-end subprocess contract (bundled scripts on disk)