mthds-6-package-manifest #670
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…CLI (Phase 2) Introduce the package manifest system for .mthds bundles: MthdsPackageManifest data model with TOML parsing/serialization, walk-up manifest discovery, cross-domain pipe visibility enforcement against [exports], cross-package -> reference detection, CLI commands (pipelex pkg init/list), and builder awareness for multi-domain output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ackage manifest Make description a required field on MthdsPackageManifest (no longer optional), add version constraint validation for dependencies (supporting ^, ~, >=, <=, >, <, ==, !=, comma-separated, and wildcard syntax), and update all test fixtures and invalid manifest files to include the now-required description field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
New pages: packages.md (manifest reference, exports/visibility, dependencies, directory structure, quick start) and pkg.md (CLI reference for pkg init/list). Updated domain.md with hierarchical domains section, project-organization.md with METHODS.toml in project tree, CLI index with pkg command row, and mkdocs.yml nav entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…date_all Add a step-by-step manual testing guide (refactoring/testing-package-system.md) with ready-to-use .mthds fixture files covering local visibility enforcement and cross-package reference syntax. Mark test_validate_all as xfail since the test fixtures contain intentional visibility violations for testing purposes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 53b1b62d0a
ℹ️ 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".
… file discovery order) On Linux/GHA, file discovery order differs from macOS: the visibility check may not find the fixture METHODS.toml, causing a DomainLibraryError from duplicate 'scoring' domain instead of the LibraryLoadingError seen locally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Non-table dependency values (e.g. `foo = "1.0.0"`) were silently dropped during parsing, causing confusing errors later during alias resolution. Now raises ManifestValidationError immediately with a clear message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The export-building loop in do_pkg_init was using sorted(pipe_codes) and ignoring the populated domain_main_pipes dict, making it dead code. Now matches the sibling pattern in builder_loop.py by placing main_pipe first in each domain's export list before appending remaining pipes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deduplicate ~35 lines of identical .mthds scanning and DomainExports building logic from builder_loop.py and init_cmd.py into two shared functions in pipelex/core/packages/bundle_scanner.py, reducing divergence risk when fixing or extending this code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…domain When two .mthds bundles share the same domain but declare different main_pipe values, the scanner now reports the conflict in the errors list instead of silently overwriting. The first value is kept for determinism. Identical main_pipe declarations across bundles are allowed. A secondary warning guard is added in PackageVisibilityChecker for defense in depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ManifestValidationError The exports parsing path in parse_methods_toml was missing the same ValidationError handling that the dependency parsing path already had, causing unhandled pydantic ValidationError to escape to callers expecting ManifestError. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lchoquel
changed the title
…_all The refactoring/test-package-fixtures/ directory contained intentionally non-compliant .mthds bundles (visibility violations, domain name collisions) that caused LibraryLoadingError/DomainLibraryError when test_validate_all scanned from ".". This forced a blanket xfail that hid real regressions. Equivalent, properly namespaced fixtures already exist in tests/data/packages/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| pipes_value = value_dict["pipes"] | ||
| if isinstance(pipes_value, list): | ||
| pipes_list = cast("list[str]", pipes_value) | ||
| result.append(DomainExports(domain_path=current_path, pipes=pipes_list)) |
There was a problem hiding this comment.
Silent data loss for non-list pipes in exports
Low Severity
In _walk_exports_table, when pipes exists in a domain's dict but is not a list (e.g., user writes pipes = "single_pipe" instead of pipes = ["single_pipe"]), the domain's export is silently dropped. The isinstance(pipes_value, list) guard prevents the DomainExports model from being instantiated, bypassing its Pydantic validation entirely. This means no error is raised — the domain simply has no exports, and its pipes are treated as private. An explicit error when pipes is present but not a list would avoid confusing silent behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
refactoring/testing-package-system.md) covering local visibility enforcement,pkg list,pkg init,main_pipeauto-export, backward compatibility, and cross-package->syntaxrefactoring/test-package-fixtures/) with three domains (legal, scoring, reporting) including intentional visibility violations for testing pass/fail scenariostest_validate_allasxfail(raises=LibraryLoadingError)since fixtures contain intentional visibility violationsTest plan
make agent-checkpasses (lint, pyright, mypy)make agent-testpasses (full test suite)pipelex pkg listfrom fixtures directory displays correct tablespipelex validate --all --library-dir refactoring/test-package-fixturescorrectly raisesLibraryLoadingErroron the blockedscoring.internal_score_normalizerreftest_validate_allshows as XFAIL with correct reason🤖 Generated with Claude Code
Note
Medium Risk
Changes bundle-loading behavior by introducing new validation that can block previously-working cross-domain pipe references when
METHODS.tomlis present. While opt-in, it touches core loading paths and introduces new parsing/validation logic that could affect developer workflows.Overview
Adds an opt-in package manifest (
METHODS.toml) with a validated data model plus TOML parsing/serialization, and a walk-up discovery mechanism (stopping at.git) to associate bundles with the nearest manifest.Enforces cross-domain pipe visibility when a manifest is present: pipes become private-by-default unless exported via
[exports](withmain_pipetreated as auto-exported). This is checked during library loading and produces actionable errors for blocked references; preliminary support foralias->...cross-package ref detection is added but not yet enforced end-to-end.Extends the CLI with a new
pipelex pkggroup (initto scaffold a manifest from discovered.mthdsfiles;listto display manifest contents), and updates the builder to optionally auto-generate a manifest in multi-domain output directories. Documentation and MkDocs nav are updated to describe packages and hierarchical domains, and a comprehensive test suite plus on-disk fixtures are added to cover manifests, discovery, visibility rules, and CLI behavior.Written by Cursor Bugbot for commit a44ba86. This will update automatically on new commits. Configure here.