refactor(makefile): parameterize target families and add deprecation framework by jonpspri · Pull Request #3353 · IBM/mcp-context-forge

jonpspri · 2026-03-01T17:06:32Z

Summary

Consolidate near-duplicate Makefile target families into parameterized versions with variables, reducing ~120 lines of boilerplate
Add is_true and deprecated_target helper macros with per-target removal version (v1.2.0)
Introduce # deprecated: tag system that make help renders as a highlighted section (dim yellow) at the end of help output
Convert impacted targets (black, isort, ruff, pylint, future-proof-ruff) from $(VENV_DIR)/bin/<tool> to uv run
Update documentation (tuning.md, ADR-025, DEVELOPING.md) to reference new parameterized invocations
Add docker-nuke target for full container environment cleanup (stops all containers, removes images, volumes, networks, and build cache)
Add Claude Code skills for pr-review, pr-risk-scoring, and sprint-plan
Update .gitignore to track .claude/skills/ directory

Targets consolidated

Family	Before	After	Mechanism
black / isort	4 targets	2 parameterized + 2 deprecated stubs	`CHECK=1` variable
ruff	4 targets	1 parameterized + 3 deprecated stubs	`RUFF_MODE=check\|fix\|format`
lint-changed/staged/commit	3 near-identical targets	3 thin call sites via `lint_git_files` macro	`define/endef`
test-ui-*	12 targets with repeated boilerplate	12 thin call sites via `run_playwright_test` macro	`define/endef`
container-run-*	9 targets	1 parameterized + 8 deprecated stubs	`CONTAINER_SSL`, `CONTAINER_HOST_NET`, `CONTAINER_JWT`, `CONTAINER_HTTP_SERVER`

Deprecated aliases

All old target names still work but emit a warning with the removal version:

  ⚠️  WARNING: "container-run-ssl" is deprecated. Use "make container-run CONTAINER_SSL=1" instead.
     This alias will be removed in v1.2.0.

make help now shows a dedicated deprecated targets section at the bottom.

New target: `docker-nuke`

Destructive cleanup target that removes ALL containers, images, volumes, networks, and build cache. Includes an interactive confirmation prompt before proceeding.

Claude Code skills

Added three reusable skills under .claude/skills/:

pr-review — structured code review for feature branches with rebase, security/correctness checks
pr-risk-scoring — score open PRs by risk level and generate CSV reports
sprint-plan — plan sprints based on current project issues

Test plan

Copilot

Pull request overview

This PR refactors the repository’s Makefile target “families” into parameterized targets, adds a deprecation framework (warnings + help output section), and updates docs to match the new invocations—while migrating several Python tooling targets to uv run.

Changes:

Introduces is_true / deprecated_target helpers and a # deprecated: tagging system that make help renders as a dedicated deprecated section.
Consolidates/parameterizes formatting/lint/container-run/playwright target families, keeping old names as deprecated aliases.
Updates contributor and ops documentation to reference the new parameterized make invocations.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`Makefile`	Adds deprecation/help rendering, parameterized targets (black/isort/ruff/container-run/playwright), and deprecated alias stubs.
`docs/docs/manage/tuning.md`	Updates container-run examples to the new `CONTAINER_*` variable style.
`docs/docs/architecture/adr/025-granian-http-server.md`	Updates ADR container-run examples to the new parameterized invocations.
`DEVELOPING.md`	Updates contributor docs for new black/isort/ruff usage variables.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Makefile

crivetimihai · 2026-03-01T19:34:30Z

Thanks @jonpspri. Clean consolidation — the is_true/deprecated_target macros are well-designed and the backward-compat aliases with removal versions are a nice touch. The uv run migration is consistent with our direction. One note: the # deprecated: tag rendering in make help is a good UX pattern. LGTM overall.

gandhipratik203

It could be beneficial to split the echo-delay test server changes (mcp-servers/rust/fast-test-server/src/main.rs and tests/loadtest/locustfile_echo_delay.py) into a separate PR, as they appear unrelated to the Makefile and skills-related updates.
That would allow the test infrastructure to merge quickly, while the Makefile and skills changes go through their own review cycle at their own pace.

jonpspri · 2026-03-11T09:19:46Z

It could be beneficial to split the echo-delay test server changes (mcp-servers/rust/fast-test-server/src/main.rs and tests/loadtest/locustfile_echo_delay.py) into a separate PR, as they appear unrelated to the Makefile and skills-related updates.

That would allow the test infrastructure to merge quickly, while the Makefile and skills changes go through their own review cycle at their own pace.

Normally I'd agree, but our current throughput challenge is the quantity of PRs and review cycle overhead on individual PRs. I'm trying to group my changes on to-be-merged PRs in a fashion that makes sense.

…mework, and unify pytest ignores - Consolidate black/isort/ruff/container-run/playwright target families into parameterized versions with variables (CHECK=1, RUFF_MODE=, CONTAINER_SSL=1, etc.) - Add is_true and deprecated_target helper macros with per-target removal version (v1.2.0) - Introduce # deprecated: tag system rendered in make help as highlighted section - Fix help recipe to stream grep output directly into while-read loop (preserves newlines) - Add PYTEST_IGNORE variable to unify --ignore flags across all pytest targets - Convert black/isort/ruff/pylint targets from $(VENV_DIR)/bin/<tool> to uv run - Add docker-nuke target for full container environment cleanup - Add Claude Code skills for pr-review, pr-risk-scoring, and sprint-plan - Revise pr-review skill for conciseness (183 -> 80 lines) and add structured output format - Fix playwright debug/update-snapshots targets to use fail mode instead of continue Signed-off-by: Jonathan Springer <jps@s390x.com>

…, and evict broken sessions Add a Streamable HTTP echo-with-delay Locust test to measure gateway throughput with I/O-bound backends. Parameterize docker-compose for gateway replicas, resource limits, worker count, and locustfile selection. The fast_test_server echo tool now accepts an optional delay parameter. Fix broken session recycling in MCP session pool by evicting sessions that encountered transport errors instead of returning them. Signed-off-by: Jonathan Springer <jps@s390x.com>

Cover the release(discard=True) path and context manager error handling added in the session pool eviction fix. Tests verify: - Discarded sessions are closed and not returned to pool - Semaphore is released on discard (including missing semaphore edge case) - Eviction counter increments on discard - Context manager passes discard=True on exception, discard=False on success Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai · 2026-03-12T21:15:58Z

Review Summary

Rebased onto main (clean, no conflicts) and reviewed the full diff.

Makefile Parameterization

Well-designed consolidation. The is_true, deprecated_target, lint_git_files, and run_playwright_test macros eliminate ~120 lines of boilerplate cleanly. All deprecated aliases delegate correctly. The # deprecated: tag system for make help is a nice addition. Documentation updates in DEVELOPING.md, tuning.md, and ADR-025 are consistent with the new invocations.

Session Pool Eviction Fix

Correct and important fix. The BaseException catch in the context manager properly marks failed sessions for eviction via release(discard=True) instead of recycling broken connections back into the pool. The discard path in release() correctly closes the session, releases the semaphore, and increments the eviction counter.

Added differential test coverage for the new discard behavior:

release(discard=True) closes session, releases semaphore, increments evictions
release(discard=True) handles missing semaphore (concurrent eviction edge case)
Discarded sessions are NOT returned to the pool queue
Context manager passes discard=True on exception, discard=False on success
Updated existing test to verify the discard kwarg

Docker-Compose & Load Testing

Clean parameterization of replicas, resource limits, locustfile selection, and gateway config via ${VAR:-default}. The virtual server registration script has proper retry logic. The echo-delay locustfile is well-structured.

No Issues Found

No security concerns (no auth/RBAC changes, test-only JWT key)
No performance regressions (session pool fix is net positive)
docker-nuke has interactive confirmation — appropriate for destructive target
PYTEST_IGNORE unification is correct (tests/manual contains non-automated test plans)

crivetimihai

LGTM. Rebased, reviewed, and added differential test coverage for the session pool discard behavior. All 135 session pool coverage tests pass.

…framework (#3353) * refactor(makefile): parameterize target families, add deprecation framework, and unify pytest ignores - Consolidate black/isort/ruff/container-run/playwright target families into parameterized versions with variables (CHECK=1, RUFF_MODE=, CONTAINER_SSL=1, etc.) - Add is_true and deprecated_target helper macros with per-target removal version (v1.2.0) - Introduce # deprecated: tag system rendered in make help as highlighted section - Fix help recipe to stream grep output directly into while-read loop (preserves newlines) - Add PYTEST_IGNORE variable to unify --ignore flags across all pytest targets - Convert black/isort/ruff/pylint targets from $(VENV_DIR)/bin/<tool> to uv run - Add docker-nuke target for full container environment cleanup - Add Claude Code skills for pr-review, pr-risk-scoring, and sprint-plan - Revise pr-review skill for conciseness (183 -> 80 lines) and add structured output format - Fix playwright debug/update-snapshots targets to use fail mode instead of continue Signed-off-by: Jonathan Springer <jps@s390x.com> * feat(loadtest): add echo-delay load test, parameterize docker-compose, and evict broken sessions Add a Streamable HTTP echo-with-delay Locust test to measure gateway throughput with I/O-bound backends. Parameterize docker-compose for gateway replicas, resource limits, worker count, and locustfile selection. The fast_test_server echo tool now accepts an optional delay parameter. Fix broken session recycling in MCP session pool by evicting sessions that encountered transport errors instead of returning them. Signed-off-by: Jonathan Springer <jps@s390x.com> * test: add differential coverage for session pool discard behavior Cover the release(discard=True) path and context manager error handling added in the session pool eviction fix. Tests verify: - Discarded sessions are closed and not returned to pool - Semaphore is released on discard (including missing semaphore edge case) - Eviction counter increments on discard - Context manager passes discard=True on exception, discard=False on success Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Jonathan Springer <jps@s390x.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>

…framework (IBM#3353) * refactor(makefile): parameterize target families, add deprecation framework, and unify pytest ignores - Consolidate black/isort/ruff/container-run/playwright target families into parameterized versions with variables (CHECK=1, RUFF_MODE=, CONTAINER_SSL=1, etc.) - Add is_true and deprecated_target helper macros with per-target removal version (v1.2.0) - Introduce # deprecated: tag system rendered in make help as highlighted section - Fix help recipe to stream grep output directly into while-read loop (preserves newlines) - Add PYTEST_IGNORE variable to unify --ignore flags across all pytest targets - Convert black/isort/ruff/pylint targets from $(VENV_DIR)/bin/<tool> to uv run - Add docker-nuke target for full container environment cleanup - Add Claude Code skills for pr-review, pr-risk-scoring, and sprint-plan - Revise pr-review skill for conciseness (183 -> 80 lines) and add structured output format - Fix playwright debug/update-snapshots targets to use fail mode instead of continue Signed-off-by: Jonathan Springer <jps@s390x.com> * feat(loadtest): add echo-delay load test, parameterize docker-compose, and evict broken sessions Add a Streamable HTTP echo-with-delay Locust test to measure gateway throughput with I/O-bound backends. Parameterize docker-compose for gateway replicas, resource limits, worker count, and locustfile selection. The fast_test_server echo tool now accepts an optional delay parameter. Fix broken session recycling in MCP session pool by evicting sessions that encountered transport errors instead of returning them. Signed-off-by: Jonathan Springer <jps@s390x.com> * test: add differential coverage for session pool discard behavior Cover the release(discard=True) path and context manager error handling added in the session pool eviction fix. Tests verify: - Discarded sessions are closed and not returned to pool - Semaphore is released on discard (including missing semaphore edge case) - Eviction counter increments on discard - Context manager passes discard=True on exception, discard=False on success Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Jonathan Springer <jps@s390x.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: KRISHNAN, SANTHANA <sk8069@exo.att.com>

Copilot AI review requested due to automatic review settings March 1, 2026 17:06

jonpspri requested a review from crivetimihai as a code owner March 1, 2026 17:06

Copilot started reviewing on behalf of jonpspri March 1, 2026 17:07 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

Makefile Show resolved Hide resolved

crivetimihai added this to the Release 1.1.0 milestone Mar 1, 2026

crivetimihai added chore Linting, formatting, dependency hygiene, or project maintenance chores SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels Mar 1, 2026

jonpspri force-pushed the refactor/makefile-parameterize-target-families branch 10 times, most recently from 1246ad2 to 1a4dd2e Compare March 6, 2026 09:37

jonpspri mentioned this pull request Mar 6, 2026

[BUG][PERFORMANCE]: MCP session pool recycles broken sessions, causing cascading ClosedResourceError failures under load #3520

Closed

jonpspri requested review from kevalmahajan and madhav165 as code owners March 6, 2026 19:29

jonpspri added the release-fix Critical bugfix required for the release label Mar 6, 2026

gandhipratik203 self-requested a review March 9, 2026 15:18

gandhipratik203 requested changes Mar 9, 2026

View reviewed changes

gandhipratik203 mentioned this pull request Mar 11, 2026

fix(session-pool): prevent broken session recycling in MCPSessionPool #3605

Closed

10 tasks

jonpspri force-pushed the refactor/makefile-parameterize-target-families branch from e642b40 to b296c38 Compare March 11, 2026 09:10

jonpspri force-pushed the refactor/makefile-parameterize-target-families branch 3 times, most recently from 40d7c96 to 69def2e Compare March 12, 2026 06:49

jonpspri and others added 3 commits March 12, 2026 21:04

crivetimihai force-pushed the refactor/makefile-parameterize-target-families branch from 69def2e to b2ca0b2 Compare March 12, 2026 21:15

crivetimihai approved these changes Mar 12, 2026

View reviewed changes

crivetimihai merged commit cbb6720 into main Mar 12, 2026
32 checks passed

crivetimihai deleted the refactor/makefile-parameterize-target-families branch March 12, 2026 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(makefile): parameterize target families and add deprecation framework#3353

refactor(makefile): parameterize target families and add deprecation framework#3353
crivetimihai merged 3 commits intomainfrom
refactor/makefile-parameterize-target-families

jonpspri commented Mar 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crivetimihai commented Mar 1, 2026

Uh oh!

gandhipratik203 left a comment

Uh oh!

jonpspri commented Mar 11, 2026

Uh oh!

crivetimihai commented Mar 12, 2026

Uh oh!

crivetimihai left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jonpspri commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Targets consolidated

Deprecated aliases

New target: docker-nuke

Claude Code skills

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crivetimihai commented Mar 1, 2026

Uh oh!

gandhipratik203 left a comment

Choose a reason for hiding this comment

Uh oh!

jonpspri commented Mar 11, 2026

Uh oh!

crivetimihai commented Mar 12, 2026

Review Summary

Makefile Parameterization

Session Pool Eviction Fix

Docker-Compose & Load Testing

No Issues Found

Uh oh!

crivetimihai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpspri commented Mar 1, 2026 •

edited

Loading

New target: `docker-nuke`