ci: build and test dependency bumps before they can be merged - #139
Conversation
Dependabot PRs passed on the Ruff job alone, which never reads package.json, the lockfile, or pyproject.toml. The console build and the test suite both lived in publish-mcp.yml, which runs on a version tag, so a broken bump was found after the tag was already spent. PR #136 (@vitejs/plugin-react 6, which needs vite 8) showed three green checks and fails the console build. - console.yml: install --frozen-lockfile, typecheck, build, on pull requests. - python.yml: server.json validation, the mypy baseline gate, and the suite (1086 tests). Push to MARM-main or release/** runs the type gate only; the suite is reserved for pull requests, where it can block a merge. - publish-mcp.yml: the release gate now runs the typecheck gate and Ruff, and pins both tools. It was already installing them and invoking neither. Neither new workflow filters paths at the trigger. A path-filtered workflow never starts on an unrelated PR, so a required status check would wait forever and block it. Each job runs every time and decides internally whether there is work, which keeps both eligible to be required checks. mypy and Ruff are pinned everywhere they run. scripts/typecheck.py gates on an absolute error count and Ruff promotes rules between minor releases, so an unpinned tool fails a PR whose code did not change. Ruff 0.16.2 did exactly that on PR #138. scripts/check-console-pr.py answers "is this console PR safe to merge" locally: it builds refs/pull/N/merge in a throwaway worktree and compares the emitted asset hashes against the base branch.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
📝 WalkthroughWalkthroughAdded Python and Console CI workflows with conditional execution. Strengthened MCP publishing validation with pinned tools and new gates. Added an isolated console PR checker that compares generated assets. Improved mirror recovery, platform guards, and initial indexing behavior. ChangesRepository validation and runtime reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This PR adds dependency-focused CI coverage and reports successful local validation; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c6e5666aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| python -m pip install --upgrade pip | ||
| pip install -r marm-mcp-server/requirements.txt | ||
| pip install -e './marm-mcp-server' --no-deps | ||
| pip install pytest pytest-asyncio pytest-cov jsonschema requests mypy==2.1.0 |
There was a problem hiding this comment.
On a clean runner, this command installs the runtime psutil package but omits types-psutil, even though pyproject.toml deliberately relies on those external stubs and the package imports psutil in three checked modules. Mypy therefore adds import-untyped diagnostics to the two-error baseline and fails every relevant Python CI run; the identical install list in publish-mcp.yml also blocks every release at its newly added typecheck gate.
Useful? React with 👍 / 👎.
| pull-requests: read | ||
|
|
||
| concurrency: | ||
| group: console-${{ github.head_ref || github.ref }} |
There was a problem hiding this comment.
For pull requests, github.head_ref is only the source branch name, so two PRs from different forks that both use a common name such as main share this concurrency group. A run for one PR can consequently cancel the other PR's required Console check and leave that unrelated PR blocked; include the PR number or head repository identity in this group, and make the equivalent change in python.yml.
Useful? React with 👍 / 👎.
| if proc.returncode == 0: | ||
| return local, suffix |
There was a problem hiding this comment.
When the merge ref is absent because a PR does not merge cleanly, this fallback returns the head ref as an ordinary successful target. If that head builds, main() prints SAFE and exits zero despite the label already saying it does not merge cleanly, so callers can treat a known-unmergeable PR as safe; propagate the fallback state as a failed verdict instead.
Useful? React with 👍 / 👎.
| ).stdout.split(): | ||
| run(["git", "branch", "-D", ref], REPO_ROOT, 60) |
There was a problem hiding this comment.
Whenever a user already has any local branch under console-check/, cleanup force-deletes it even if this invocation did not create it. That can irreversibly discard unrelated local commits merely by running the helper; track the specific temporary refs created during this execution and delete only those.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/console.yml:
- Around line 27-28: Disable persisted checkout credentials for the
actions/checkout steps in .github/workflows/console.yml lines 27-28 and
.github/workflows/python.yml lines 40-41 by setting persist-credentials to
false; leave the existing GH_TOKEN-based gh pr diff behavior unchanged.
In `@scripts/check-console-pr.py`:
- Around line 65-68: Update scripts/check-console-pr.py lines 65-68 to fetch PR
refs into a private non-branch namespace instead of refs/heads/console-check.
Update lines 122-127 to track and delete only the exact temporary refs created
by this invocation, preserving unrelated local console-check branches.
- Around line 66-71: Update scripts/check-console-pr.py lines 66-71 in the
ref-fetch helper to return an explicit mergeability flag alongside the fetched
ref, marking the merge ref as mergeable and the head-only fallback as not
mergeable. Retain that flag in each target record at lines 156-160, then update
the final verdict logic at lines 179-192 to force UNSAFE whenever the PR lacks a
merge ref, even if the head-only build succeeds.
- Line 150: Update the baseline-fetch logic around run to check its result and
fail when git fetch does not succeed. Fetch explicitly into
refs/remotes/origin/MARM-main so the baseline is refreshed rather than silently
reusing a stale origin/MARM-main reference.
- Around line 52-53: Update tool_missing() to validate the installed Node and
pnpm versions in addition to executable presence, requiring Node 24 and pnpm 10
to match .github/workflows/console.yml. Ensure mismatched or unavailable
versions are reported as missing before the validator can return SAFE, while
preserving checks for gh and git.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f76cfae2-8123-411c-87bd-6b724b6e812d
📒 Files selected for processing (4)
.github/workflows/console.yml.github/workflows/publish-mcp.yml.github/workflows/python.ymlscripts/check-console-pr.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
⚠️ CI failures not shown inline (2)
GitHub Actions: Python CI / 0_python.txt: ci: build and test dependency bumps before they can be merged
Conclusion: failure
##[group]Run python scripts/typecheck.py
�[36;1mpython scripts/typecheck.py�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
##[error]type errors increased: 12 > baseline 2. Fix the new errors, or run with --raw to see them in full.
GitHub Actions: Python CI / python: ci: build and test dependency bumps before they can be merged
Conclusion: failure
##[group]Run python scripts/typecheck.py
�[36;1mpython scripts/typecheck.py�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.11.15/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.11.15/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.11.15/x64/lib
##[endgroup]
##[error]type errors increased: 12 > baseline 2. Fix the new errors, or run with --raw to see them in full.
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,json}
📄 CodeRabbit inference engine (AGENTS.md)
MCP tools must remain in exact parity between HTTP and STDIO transports. HTTP tools must be registered in
server.pyandMCP_TOOL_OPERATIONS; STDIO tools must be registered throughserver_stdio.pyor its graph service path.
Files:
scripts/check-console-pr.py
**/*.{py,md,json}
📄 CodeRabbit inference engine (AGENTS.md)
Tests and documentation must cover both transports and maintain complete tool lists when MCP tools change.
Files:
scripts/check-console-pr.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use SemVer: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
scripts/check-console-pr.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
scripts/check-console-pr.py
🪛 ast-grep (0.45.1)
scripts/check-console-pr.py
[error] 41-48: Command coming from incoming request
Context: subprocess.run(
[resolved, *cmd[1:]],
cwd=cwd,
capture_output=True,
text=True,
timeout=timeout,
shell=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 zizmor (1.29.0)
.github/workflows/console.yml
[warning] 27-28: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/python.yml
[warning] 40-45: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 80-80: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🔇 Additional comments (5)
.github/workflows/python.yml (2)
1-39: LGTM!
43-120: LGTM!.github/workflows/console.yml (2)
1-26: LGTM!
30-75: LGTM!.github/workflows/publish-mcp.yml (1)
59-77: LGTM!
| def tool_missing() -> list[str]: | ||
| return [t for t in ("node", "pnpm", "gh", "git") if shutil.which(t) is None] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node_major="$(node --version | sed -E 's/^v([0-9]+).*/\1/')"
pnpm_major="$(pnpm --version | sed -E 's/^([0-9]+).*/\1/')"
test "$node_major" = "24"
test "$pnpm_major" = "10"Repository: Lyellr88/marm-memory
Length of output: 321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files 'scripts/check-console-pr.py' '.github/workflows/console.yml' '*server.py' '*server_stdio.py' | sed -n '1,120p'
printf '%s\n' '--- validator outline ---'
ast-grep outline scripts/check-console-pr.py
printf '%s\n' '--- validator sections ---'
nl -ba scripts/check-console-pr.py | sed -n '1,230p'
printf '%s\n' '--- workflow toolchain ---'
nl -ba .github/workflows/console.yml | sed -n '35,85p'Repository: Lyellr88/marm-memory
Length of output: 1083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- validator ---'
sed -n '1,230p' scripts/check-console-pr.py
printf '%s\n' '--- workflow ---'
sed -n '35,85p' .github/workflows/console.yml
printf '%s\n' '--- package manager and runtime declarations ---'
rg -n --hidden -g '!node_modules' -g '!dist' -g '!build' \
'node-version|pnpm/action-setup|packageManager|engines|corepack|pnpm' \
.github package.json marm-console 2>/dev/null | sed -n '1,220p'Repository: Lyellr88/marm-memory
Length of output: 22100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
validator = Path("scripts/check-console-pr.py").read_text()
workflow = Path(".github/workflows/console.yml").read_text()
tree = ast.parse(validator)
tool_missing = next(
node for node in ast.walk(tree)
if isinstance(node, ast.FunctionDef) and node.name == "tool_missing"
)
calls = [
node for node in ast.walk(tool_missing)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "which"
]
assert len(calls) == 1, "tool_missing() does not have the expected executable-presence check"
call = calls[0]
assert ast.literal_eval(call.args[0]) == "t" if False else True
source = ast.get_source_segment(validator, tool_missing)
assert source is not None
assert '"node"' in source and '"pnpm"' in source
assert "version" not in source and "--version" not in source
assert "node-version: '24'" in workflow
assert "version: '10'" in workflow
assert '["pnpm", "install", "--frozen-lockfile"]' in validator
assert '["pnpm", "typecheck"]' in validator
assert '["pnpm", "build"]' in validator
print("tool_missing() checks executable presence only: PASS")
print("CI declares Node major 24 and pnpm major 10: PASS")
print("validator runs install, typecheck, and build through pnpm: PASS")
PY
printf '%s\n' '--- available local tool versions ---'
for tool in node pnpm; do
if command -v "$tool" >/dev/null 2>&1; then
"$tool" --version
else
printf '%s unavailable\n' "$tool"
fi
doneRepository: Lyellr88/marm-memory
Length of output: 543
Require the CI toolchain before returning SAFE.
tool_missing() checks only executable presence. Require Node 24 and pnpm 10, matching .github/workflows/console.yml, before running the build. Otherwise, the validator can report SAFE with a toolchain that differs from CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-console-pr.py` around lines 52 - 53, Update tool_missing() to
validate the installed Node and pnpm versions in addition to executable
presence, requiring Node 24 and pnpm 10 to match .github/workflows/console.yml.
Ensure mismatched or unavailable versions are reported as missing before the
validator can return SAFE, while preserving checks for gh and git.
Source: Path instructions
The new Python job failed on its first run: 12 errors against a baseline of 2. The baseline was recorded on Windows and CI is Linux, and two separate things made the counts diverge. Platform narrowing. mypy narrows platform on sys.platform and not on os.name, so `if os.name == "nt"` branches were analyzed on Linux, where subprocess.CREATE_NEW_PROCESS_GROUP and DETACHED_PROCESS do not exist. Switched the two guards that gate those flags. Equivalent at runtime. _set_windows_owner_only_dacl had no platform guard of its own and dereferences ctypes.WinDLL, so it now returns False off Windows rather than relying on its caller's check; a direct call elsewhere would have raised AttributeError. Missing stubs. types-psutil is declared in the dev extra, but both workflows install their test tools explicitly and never picked it up, so psutil resolved to Any in CI and produced three import-untyped errors plus a no-any-return. Added to both install lines. mypy now reports the same 2 errors natively and under --platform linux, so the baseline means the same thing on a developer machine and on the runner. runtime_manager.py:370 keeps os.name deliberately: its branches are valid on both platforms and its SIGKILL ignore depends on mypy checking the else branch.
_Watched.last_full started at 0.0 and is compared as
`monotonic() - last_full < GRAPH_AUTO_INDEX_FULL_INTERVAL`. monotonic() counts
from boot, so 0.0 reads as "indexed at boot" rather than "never indexed": on a
machine up for less than the 300s interval, a non-git project's first index
waited the interval out instead of running immediately. float("-inf") states
"never" in the same units the comparison uses.
Found by the new Python CI job, which failed five tests in
test_graph_auto_index.py that pass locally. They were not asserting anything
wrong; their outcome depended on the host's uptime, verified directly:
monotonic()=12s -> reindexed=[] (fresh CI runner)
monotonic()=299s -> reindexed=[]
monotonic()=301s -> reindexed=['non_git_interval']
The release workflow hid this. It spends minutes on model bundling, a pnpm
install, a console build, and a Docker build before pytest, so the runner was
always past 300s uptime by then. The new job installs and tests directly, which
starts pytest early enough in the VM's life to expose it. Not a regression from
the v2.38.0 typing pass, which touched this file for annotations only.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Concurrency keyed on the PR number rather than head_ref. Two PRs from different forks can both use a branch named `main`, and a shared group means one cancels the other's required check, leaving that PR blocked with nothing to report. persist-credentials: false on both checkouts. The console job runs pnpm install on a PR-controlled lockfile, which executes lifecycle scripts, and the default leaves the job token in .git/config where they can read it. Both jobs pass GH_TOKEN to gh explicitly, so detection is unaffected. Pin mypy, types-psutil, and ruff in the dev extra to the versions the gates pin. The extra floored them, so `pip install -e ".[dev]"` resolved a different checker than CI and than the one that set BASELINE. Ruff was the same defect already observed live: 0.16.2 promoted RUF036 out of preview and failed a PR whose code had not changed while local 0.15.16 called it clean. check-console-pr.py: a PR with no merge ref no longer reports SAFE after its head builds, scratch refs move to refs/console-check/ and cleanup deletes only the ones the run created (it force-deleted the whole namespace, including a developer's own branches), and the baseline fetch result is checked so a failed fetch cannot build against a stale base and report the drift as PR breakage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ess call No behavior change; False is the default. The helper reads returncode itself, so saying so silences PLW1510 when ruff is run from the repo root. Root-level ruff is not a CI gate: both ruff jobs scope to marm-mcp-server, and the root scripts/ tree carries 117 findings on MARM-main that nothing scans. Untouched here rather than swept into a CI PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects in the doc mirror path, all found by review of the v2.38.0 fix and each reproduced by a failing test before being changed. Resolve by doc_id whenever the linked id fails to match a row, not only when the caller passed none. A non-null but deleted docs.memory_id skipped the lookup, so a doc that already carried duplicates gained a third row on the next save. Order that resolve by id rather than timestamp. The write rewrites the timestamp it was ordering on, so each save picked whichever duplicate it had not just touched; with a link that kept failing, saves alternated between two rows indefinitely and neither ever held current content. id never changes. Report the mirror row that was written, not the doc's stored link, whenever a row exists. Both pending causes reported the link, which was correct while pending only meant the mirror write failed and no row existed. Now that a written mirror can go unlinked, that answer returns null or a deleted id at exactly the moment a real mirror is sitting there orphaned. Null now means only that the mirror write itself failed. Pre-existing duplicates are still not removed. Deleting them takes their chunks, queue entries, and concept provenance with them, which is a migration and not done silently; saves now converge on one of them instead of spreading across them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The v2.38.0 entry covered the doc mirror work but not the auto-index bug that shipped alongside it: a never-indexed project waited out the full 300s interval before its first index whenever the host had been up for less than that, because the "last indexed" marker started at 0.0 and time.monotonic() counts from boot. Fresh containers and just-rebooted machines were affected. Developer notes gain the platform-independent type baseline, the dev extra now pinning mypy, its stubs, and Ruff to the gate versions rather than flooring them, and pull requests now building the Console and running the Python suite instead of leaving both to a version tag. No version bump: v2.38.0 is merged and untagged, so this folds into it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
CI depth for dependency bumps
Dependabot PRs were passing on the Ruff job alone, which never reads
package.json, the lockfile, orpyproject.toml. The console build and the test suite both lived inpublish-mcp.yml, which runs on a version tag, so a broken bump surfaced only after the tag was already spent. PR #136 is the live example: three green checks, and it fails the console build.console.ymlrunspnpm install --frozen-lockfile,typecheck, andbuildon pull requests.python.ymlrunsserver.jsonvalidation, the mypy baseline gate, and the suite (1086 tests). A push toMARM-mainorrelease/**runs the type gate only; the suite is reserved for pull requests, where it can block a merge.scripts/typecheck.pygates on an absolute error count and Ruff promotes rules between minor releases, so an unpinned tool fails a PR whose code did not change. Ruff 0.16.2 did exactly that on Release/v2.38.0 #138.scripts/check-console-pr.pyanswers the same question locally, buildingrefs/pull/N/mergein a throwaway worktree and comparing emitted asset hashes against the base branch.Neither new workflow filters paths at the trigger. A path-filtered workflow never starts on an unrelated PR, so a required status check would sit at "waiting to be reported" and block that PR permanently. Each job runs every time and decides internally whether there is work, keeping both eligible to be required checks.
Validation
Every gate was run locally with the exact command CI uses: typecheck
OK 2 errors, unchanged from baseline;ruff checkclean over 205 files;pytest -m "not docker and not smoke_lifecycle"at 1086 passed, 2 skipped. Path detection was checked against four real PR shapes (docs-only, this PR, a Dependabot pip bump, and #136).Upgrade Note
MARM-mainnow requires theruffcheck. Once this merges, promote the two new checks:Promoting them before this merges would block this PR, since the checks would not yet exist.
Summary by CodeRabbit