fix(release): harden repository readiness contracts - #158
Conversation
- validate complete reference packages before publishing generated artifacts - bound initializer optimization commands and report actionable failures - add pinned Markdown validation to local and CI workflows - reconcile archival guidance, API contracts, and successor issue routing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe pull request updates repository documentation, adds pinned Markdown validation, hardens reference-fixture publication, adds subprocess timeouts and concise diagnostics, and extends regression coverage. ChangesRepository validation and release workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant generate_reference_fixtures
participant validate_reference_fixtures
participant publish_artifacts
generate_reference_fixtures->>validate_reference_fixtures: validate bounded transcript
generate_reference_fixtures->>validate_reference_fixtures: validate staged package
validate_reference_fixtures-->>generate_reference_fixtures: validation result
generate_reference_fixtures->>publish_artifacts: publish validated artifacts
sequenceDiagram
participant optimize_initialize
participant Git
participant initializer_process
optimize_initialize->>Git: collect provenance with timeout and stderr capture
optimize_initialize->>initializer_process: run initializer with configured timeout
initializer_process-->>optimize_initialize: output or subprocess failure
optimize_initialize-->>optimize_initialize: format concise diagnostic
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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 `@CHANGELOG.md`:
- Around line 106-109: Update the “Enforce portable checkpoint continuation”
changelog entry to use the defined checkpoint portability terminology and
explicitly state that continuation is limited to the recorded producer
toolchain, or revise it to match the documented release limitation that rejects
portable interchange.
In `@include/Foliated_triangulation.hpp`:
- Around line 1637-1638: Update the zero-radius handling in
does_vertex_radius_match_timevalue() so an expected squared radius of zero
accepts the exact origin using an absolute-tolerance check, while preserving the
existing comparison for positive radii. Then retain the nonnegative
initial_radius precondition in the constructors documented at
include/Foliated_triangulation.hpp:1637-1638 and :1716-1717, and expose the same
nonnegative contract in include/Manifold.hpp:161-162 only with that invariant
fix applied.
In `@scripts/optimize_initialize.py`:
- Around line 306-315: Update _format_subprocess_failure to compute and append
the existing stderr/output suffix before handling the timeout-specific message.
Ensure TimeoutExpired includes any partial output while preserving the timeout
duration and command details, and retain the current exit-status formatting for
CalledProcessError.
In `@scripts/tests/test_justfile_discoverability.py`:
- Around line 128-133: Update
test_debug_cli_excludes_the_assertion_incompatible_quickstart to read
CMakePresets.json and assert that the debug-cli CTest filter excludes the exact
label pattern ^debug-incompatible$. Keep the existing label assertion, and
verify the preset configuration itself rather than relying only on the example’s
LABELS declaration.
In `@scripts/validate_reference_fixtures.py`:
- Around line 622-630: Update the pass-count validation around pass_records to
match either expected record as a complete output line rather than allowing
substring prefixes; preserve the existing accepted wording while rejecting
values such as 10 when passes is 1. Add a regression case that changes the
retained pass count to 10 and verifies validation fails.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 20105670-b1fa-490e-8877-4e9c64829133
📒 Files selected for processing (19)
.github/CODE_OF_CONDUCT.md.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.md.github/workflows/ci.ymlCHANGELOG.mdJustfilecliff.tomlexamples/CMakeLists.txtinclude/Foliated_triangulation.hppinclude/Manifold.hpppyproject.tomlscripts/generate_reference_fixtures.pyscripts/optimize_initialize.pyscripts/tests/test_generate_reference_fixtures.pyscripts/tests/test_justfile_discoverability.pyscripts/tests/test_optimize_initialize.pyscripts/tests/test_validate_reference_fixtures.pyscripts/validate_reference_fixtures.py
💤 Files with no reviewable changes (1)
- .github/ISSUE_TEMPLATE/feature_request.md
Serialize repository-relative producer paths with stable POSIX separators so Windows regeneration satisfies the platform-neutral manifest contract.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
=======================================
Coverage 92.35% 92.35%
=======================================
Files 19 19
Lines 3990 3992 +2
Branches 1908 1909 +1
=======================================
+ Hits 3685 3687 +2
Misses 305 305
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
- accept origin vertices when the expected foliation radius is zero - retain partial timeout output and require exact fixture pass counts - align checkpoint wording with producer-toolchain limitations
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_generate_reference_fixtures.py (1)
145-148: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for a non-empty staged package.
validate_generated_package({})checks the committed-tree baseline. It does not prove that a generated artifact replaces the committed copy during validation. Add a test with one conflicting generated artifact and assert that validation fails beforepublish_artifacts.🤖 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/tests/test_generate_reference_fixtures.py` around lines 145 - 148, The test test_staged_package_validator_accepts_the_committed_tree currently covers only an empty staged package; add a separate case with one generated artifact conflicting with the committed copy, invoke validation through the relevant generator flow, and assert it fails before publish_artifacts is called.
🤖 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 `@scripts/optimize_initialize.py`:
- Around line 309-312: Update the diagnostic construction around detail in the
subprocess error-handling path to decode and preserve every non-empty value from
both error.stderr and error.output, rather than selecting only one; retain the
existing whitespace-safe suffix behavior. Add regression coverage for cases
where both captured streams contain diagnostics, including the relevant
CalledProcessError or TimeoutExpired scenario.
---
Outside diff comments:
In `@scripts/tests/test_generate_reference_fixtures.py`:
- Around line 145-148: The test
test_staged_package_validator_accepts_the_committed_tree currently covers only
an empty staged package; add a separate case with one generated artifact
conflicting with the committed copy, invoke validation through the relevant
generator flow, and assert it fails before publish_artifacts is called.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d46a6197-cda6-4fd0-852e-3adb63617998
📒 Files selected for processing (10)
CHANGELOG.mdinclude/Foliated_triangulation.hppscripts/generate_reference_fixtures.pyscripts/optimize_initialize.pyscripts/tests/test_generate_reference_fixtures.pyscripts/tests/test_justfile_discoverability.pyscripts/tests/test_optimize_initialize.pyscripts/tests/test_validate_reference_fixtures.pyscripts/validate_reference_fixtures.pytests/Foliated_triangulation_test.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_generate_reference_fixtures.py (1)
145-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the injected root or correct the docstring.
The test does not create or pass a temporary root. It calls
generator.validate_generated_package({})and therefore uses the default repository root.If injected-root behavior is the intended contract, create a temporary staged tree and pass that root. Otherwise, change the docstring to state that the test covers the committed tree only.
🤖 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/tests/test_generate_reference_fixtures.py` around lines 145 - 149, The test’s docstring claims temporary-root validation, but test_staged_package_validator_accepts_the_committed_tree calls validate_generated_package with an empty mapping and uses the default repository root. Either construct a temporary staged tree and pass its root to validate_generated_package, or revise the docstring to accurately describe validation of the committed tree.
🤖 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.
Outside diff comments:
In `@scripts/tests/test_generate_reference_fixtures.py`:
- Around line 145-149: The test’s docstring claims temporary-root validation,
but test_staged_package_validator_accepts_the_committed_tree calls
validate_generated_package with an empty mapping and uses the default repository
root. Either construct a temporary staged tree and pass its root to
validate_generated_package, or revise the docstring to accurately describe
validation of the committed tree.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb767a86-b031-4694-b736-65f3d7882bad
📒 Files selected for processing (1)
scripts/tests/test_generate_reference_fixtures.py
Combine captured stderr and stdout for initializer failures so timeout and exit-status details are not discarded.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary by CodeRabbit
Documentation
Developer Experience
Reliability