Feat/trustworthy estimate package - #1
Conversation
Close privacy, contract, lineage, failure-reporting, workbook, and runtime reliability gaps found during the compound review.
|
@copilot, please review this PR |
There was a problem hiding this comment.
Pull request overview
This PR introduces a deterministic, review-gated “estimate package” workflow across the takeoff → nest → RFQ pipeline, with machine-readable run publication (manifests + QA reports), public-data/provenance guardrails, and extensive contract/CLI/runtime tests to enforce the new semantics.
Changes:
- Adds shared runtime primitives (
pi_steel) for run publication, CLI failure diagnostics, parsing/adapters, and geometry verification. - Tightens documentation and tooling boundaries around pricing, draft-only artifacts, and public-data safety (policy + provenance gate).
- Adds comprehensive pytest/unittest coverage plus synthetic fixtures/goldens and CI jobs (base + full-render).
Reviewed changes
Copilot reviewed 87 out of 89 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_takeoff_cli.py | CLI regression tests for legacy BOM validation + weight output. |
| tests/test_structured_failure_cli.py | Ensures post-parse failures still publish machine-readable diagnostics. |
| tests/test_runtime_bootstrap.py | Verifies entrypoints run from arbitrary CWD and bootstrap works. |
| tests/test_run_manifests.py | Contract tests for run publication, hashing, allow-lists, rollback. |
| tests/test_rfq_workbook_contract.py | Workbook structure + semantic projection contract tests. |
| tests/test_rfq_generator.py | RFQ normalization, legacy adapter, profile, CLI, and nest-handoff tests. |
| tests/test_recalc.py | Tests recalc-on-open error handling and CLI messaging. |
| tests/test_public_data_policy.py | Tests public-data scanner behavior and gitignore defaults. |
| tests/test_package_contents.py | Verifies npm pack contents and release gate wiring. |
| tests/test_nest_outputs.py | Nest publication + DXF output gating/invariants tests. |
| tests/test_nest_invariants.py | Geometry verifier invariants + performance expectations. |
| tests/test_nest_engine.py | Nest engine behavioral contracts (fit, grouping, cost basis, metrics). |
| tests/test_nest_cli_contract.py | CLI contract tests for nest outcomes and artifacts. |
| tests/test_nest_burn_guard.py | Burn-DXF guard tests for suppression/eligibility conditions. |
| tests/test_installed_scripts.py | Smoke-tests packed artifact runtime and pipeline behavior. |
| tests/test_full_render_smoke.py | Optional full-render smoke suite (LibreOffice + poppler). |
| tests/test_data_provenance.py | Verifies shipped dataset integrity and release blocking. |
| tests/test_acknowledge_finding_cli.py | Tests acknowledgement helper behavior and safety checks. |
| tests/golden/rfq/semantic-workbook.json | Golden semantic projection for RFQ workbook. |
| tests/golden/pipeline/ready-artifacts.json | Golden expected artifact set for “ready” pipeline runs. |
| tests/fixtures/workflows/README.md | Documents synthetic workflow fixture provenance and intent. |
| tests/fixtures/workflows/mixed_plate_job.json | Synthetic mixed rectangular/irregular nesting workflow fixture. |
| tests/fixtures/workflows/member_only.csv | Synthetic member-only takeoff fixture. |
| tests/fixtures/workflows/blocked_plate_job.json | Synthetic blocked nesting fixture (oversize part). |
| tests/fixtures/workflows/ambiguous_scope.csv | Synthetic ambiguous-scope takeoff fixture. |
| tests/fixtures/rfq/synthetic-profile.json | Synthetic company profile fixture for RFQ tests. |
| tests/fixtures/rfq/README.md | Documents RFQ fixture provenance and constraints. |
| tests/fixtures/rfq/nest-handoff.json | Synthetic RFQ nest-handoff contract fixture. |
| tests/fixtures/rfq/estimate-package.json | Canonical estimate-package fixture for RFQ tests. |
| tests/fixtures/pipeline/synthetic-profile.json | Pipeline profile fixture (no logo). |
| tests/fixtures/pipeline/synthetic-estimate.json | Canonical estimate-package fixture for end-to-end pipeline. |
| tests/fixtures/pipeline/README.md | Documents pipeline fixture provenance and constraints. |
| tests/fixtures/nest/README.md | Documents nest fixture provenance and constraints. |
| tests/fixtures/nest/irregular-reference-only.json | Synthetic irregular reference-only nest job fixture. |
| tests/fixtures/nest/grouped-engine.json | Synthetic grouped-material nest fixture for engine contracts. |
| tests/fixtures/contracts/README.md | Documents contract fixtures provenance. |
| tests/fixtures/contracts/legacy-nest.json | Legacy nest-job fixture for adapter/contract coverage. |
| tests/fixtures/contracts/legacy-bom.csv | Legacy BOM fixture for adapter/contract coverage. |
| skills/steel-takeoff/SKILL.md | Updates BOM field contract + pricing boundary documentation. |
| skills/steel-takeoff/scripts/calculate-weight.sh | Removes pricing output; handles unspecified grade explicitly. |
| skills/steel-takeoff/references/takeoff-procedures.md | Removes market-rate language from public guide. |
| skills/steel-takeoff/assets/bom-template.csv | Updates BOM template headers to new field set. |
| skills/steel-rfq/SKILL.md | Rewrites skill doc to deterministic compiler + boundary contract. |
| skills/steel-rfq/scripts/recalc.py | Adds explicit error type + structured CLI semantics. |
| skills/steel-rfq/references/rfq-input.md | Adds explicit RFQ compiler input contract (canonical + legacy). |
| skills/steel-rfq/assets/company-profile.example.json | Replaces example profile with approval-bound terms template shape. |
| skills/steel-nest/SKILL.md | Updates nesting docs for verification, guarded burn output, manifests. |
| skills/steel-nest/references/job_template.json | Updates template to explicit material basis + synthetic public example. |
| skills/steel-nest/references/FIXTURE_PROVENANCE.md | Documents provenance for public synthetic examples. |
| skills/steel-nest/references/example_job.json | Replaces example with synthetic public geometry + explicit basis. |
| skills/steel-estimate/SKILL.md | New orchestrator documentation for review-gated pipeline publication. |
| skills/steel-estimate/scripts/acknowledge-finding.py | New CLI helper to record explicit review acknowledgements safely. |
| skills/steel-estimate/references/output-contract.md | Defines published artifact/outcome contract and readiness semantics. |
| skills/steel-estimate/references/estimate-package-example.json | Adds canonical synthetic estimate-package example. |
| skills/_shared/schemas/run-manifest.schema.json | Adds run-manifest JSON schema contract. |
| skills/_shared/pi_steel/run_manifest.py | Implements isolated run publication + hashing + pointer updates. |
| skills/_shared/pi_steel/parsing.py | Adds legacy adapters for BOM CSV + nest JSON into canonical package. |
| skills/_shared/pi_steel/geometry_verify.py | Adds independent placement verification + grouping helpers. |
| skills/_shared/pi_steel/contracts.py | Adds canonical contract constants + deterministic ID helpers. |
| skills/_shared/pi_steel/cli.py | Adds shared CLI parsing + structured failure diagnostic publication. |
| skills/_shared/pi_steel/init.py | Exposes shared runtime API surface for shipped scripts. |
| skills/_shared/bootstrap.py | Adds relocatable bootstrap to locate shipped skills tree. |
| scripts/doctor.py | Adds runtime capability/dep doctor with machine-readable output. |
| scripts/check-data-provenance.py | Adds provenance integrity + release gate check for shipped dataset. |
| requirements.txt | Adds base Python runtime dependencies. |
| requirements-tested.txt | Records exact dependency set used for tested runtime environment. |
| requirements-render.txt | Adds optional render-tier Python dependencies. |
| requirements-dev.txt | Adds dev/test dependencies. |
| README.md | Updates product boundary, safety, and dev/release check documentation. |
| pyproject.toml | Adds Python project metadata + pytest/ruff config. |
| PUBLIC_DATA_POLICY.md | Adds explicit public-data policy + fixture rules. |
| package.json | Adds lint/test/privacy/provenance/pack/release scripts and files allowlist. |
| docs/research/v0.3-workflow-evidence.md | Adds workflow evidence doc for reliability milestone. |
| DATA_PROVENANCE.md | Adds human-readable dataset provenance + release decision. |
| DATA_PROVENANCE.json | Adds machine-readable dataset provenance + checksum contract. |
| .npmignore | Ensures private/generated artifacts and local configs are excluded from npm. |
| .gitignore | Ensures private/generated artifacts and local configs are excluded from git. |
| .github/workflows/ci.yml | Adds CI for base and full-render suites across Python versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def parse_length_ft(raw: str) -> float: | ||
| value = raw.strip() | ||
| if "'" not in value: | ||
| return float(value) | ||
| feet, _, inches = value.replace('"', "").partition("'") | ||
| return float(feet.replace("-", "").strip()) + ( | ||
| float(inches.replace("-", "").strip()) / 12 if inches.strip() else 0 | ||
| ) |
| def audit() -> dict: | ||
| errors: list[str] = [] | ||
| record = json.loads(PROVENANCE_RECORD_PATH.read_text(encoding="utf-8")) | ||
| datasets = record.get("datasets", []) | ||
| if len(datasets) != 1: | ||
| errors.append("expected exactly one declared shipped dataset") | ||
| dataset = {} | ||
| else: | ||
| dataset = datasets[0] | ||
| shapes_path = ROOT / dataset.get("shipped_file", "") | ||
| raw = shapes_path.read_bytes() | ||
| checksum = hashlib.sha256(raw).hexdigest() | ||
| if checksum != dataset.get("sha256"): | ||
| errors.append("shape data checksum differs from DATA_PROVENANCE.json") | ||
|
|
||
| rows = json.loads(raw) |
Here's my review. I found three issues: 🔴 Medium —
|
|
@CodeRabbit, review this project. |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR adds a versioned, review-gated steel estimating workflow with canonical contracts, validated nesting, deterministic draft RFQ compilation, isolated run manifests, provenance and public-data checks, CI automation, and extensive synthetic integration coverage. ChangesEstimate workflow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Input
participant SteelEstimate
participant SteelNest
participant SteelRFQ
participant RunPublisher
Input->>SteelEstimate: load canonical estimate package
SteelEstimate->>SteelNest: submit eligible plate items and stock
SteelNest->>SteelEstimate: return verified nest result and RFQ handoff
SteelEstimate->>SteelRFQ: compile draft workbook from package and handoff
SteelRFQ->>RunPublisher: write workbook and semantic projection
SteelEstimate->>RunPublisher: write QA report and run manifest
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
skills/steel-takeoff/scripts/calculate-weight.sh-7-7 (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUsage help still omits
Source_ID.The header now documents
Source_IDas the first BOM column, but the usage text printed at Line 20 still lists the old column set. Worth syncing so users generating a CSV from--helpoutput produce a compatible file.🤖 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 `@skills/steel-takeoff/scripts/calculate-weight.sh` at line 7, Update the usage/help text in calculate-weight.sh to include Source_ID as the first BOM column, matching the documented CSV header and ensuring generated CSV files are compatible.skills/steel-estimate/scripts/acknowledge-finding.py-136-146 (1)
136-146: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWrap the
os.linkfailure inAcknowledgementInputError.The
output_path.exists()precheck at Line 66 leaves a race window; if the file appears in between,os.linkraisesFileExistsErrorand escapesmain's handler, printing a traceback instead of the standardAcknowledgement failed:message.🛡️ Proposed fix
try: with os.fdopen(file_descriptor, "wb") as stream: stream.write(canonical_json_bytes(package)) - os.link(temporary_name, output_path) + try: + os.link(temporary_name, output_path) + except FileExistsError as exc: + raise AcknowledgementInputError( + f"output already exists: {output_path}" + ) from exc finally:🤖 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 `@skills/steel-estimate/scripts/acknowledge-finding.py` around lines 136 - 146, Wrap the os.link call in the acknowledgement-writing flow with handling that converts FileExistsError into AcknowledgementInputError, preserving the original failure context so main’s existing handler emits the standard “Acknowledgement failed:” message. Keep the temporary-file cleanup in the finally block unchanged.skills/steel-rfq/references/rfq-input.md-22-34 (1)
22-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocumented handoff example omits the identity fields the validator requires.
validate_nest_handoff(skills/steel-rfq/scripts/generate-rfq.pyLines 529-546) comparesproject_id,revision_id, andestimate_input_hashagainst the estimate package and emitsstale_nest_handoffon mismatch. The example object here has none of them, so copying it verbatim always blocks. Add those keys to the snippet and mention the staleness rule alongside the version/boundary rules on Line 33.🤖 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 `@skills/steel-rfq/references/rfq-input.md` around lines 22 - 34, Update the versioned rfq_nesting.json example to include project_id, revision_id, and estimate_input_hash fields required by validate_nest_handoff. Add a concise note stating these identity values must match the estimate package or the handoff is rejected as stale, alongside the existing version and boundary rules.skills/_shared/pi_steel/validation.py-447-471 (1)
447-471: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
reservedon-hand stock passes validation but is silently dropped from eligible stock.Line 454 treats
status in {"available", "reserved"}as confirmed, so no blocker or warning is raised, yeteligible_on_hand_stock(Line 570) requiresstatus == "available"and excludes it. The estimator gets a cleanvalidatedpackage while the reserved inventory is silently ignored in consumption. Either warn on reserved on-hand stock or align the two predicates.🛡️ Proposed fix — surface reserved stock explicitly
- required = ( - stock.get("inventory_id"), - stock.get("measured_at"), - stock.get("source"), - stock.get("status") in {"available", "reserved"}, - ) + required = ( + stock.get("inventory_id"), + stock.get("measured_at"), + stock.get("source"), + stock.get("status") == "available", + )Or keep
reservedacceptable and emit areserved_on_hand_stockwarning so the exclusion from consumption is visible.Also applies to: 557-576
🤖 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 `@skills/_shared/pi_steel/validation.py` around lines 447 - 471, Align the on-hand stock validation near the confirmation predicate with the eligible_on_hand_stock filtering logic so reserved stock is not silently ignored. Prefer preserving reserved as valid while adding a reserved_on_hand_stock warning for confirmed reserved entries, and ensure the warning points to the stock item without changing available-stock consumption behavior.skills/steel-nest/references/job_template.json-29-29 (1)
29-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
_comment_holespromises burn-DXF geometry unconditionally.
render_burn_dxfsemits nothing unlessburn_dxf_eligibleis true, and this template itself contains anirregularpart (Part B), which suppresses burn output. Align the wording with the gate documented inskills/steel-nest/SKILL.md.📝 Proposed wording
- "_comment_holes": "Optional per-part \"holes\": each hole's x,y is its CENTER measured from the part's lower-left corner. Round hole = {\"dia\": D, \"x\":, \"y\":}. Rectangular cutout = {\"w\":, \"h\":, \"x\":, \"y\":}. Holes are subtracted from weight/cost, rotate with the part, and are cut as real geometry on the HOLES layer of the burn-table DXF.", + "_comment_holes": "Optional per-part \"holes\": each hole's x,y is its CENTER measured from the part's lower-left corner. Round hole = {\"dia\": D, \"x\":, \"y\":}. Rectangular cutout = {\"w\":, \"h\":, \"x\":, \"y\":}. Holes are subtracted from weight/cost, rotate with the part, and appear on the HOLES layer of the burn-table DXF only when the whole job passes the burn gate (all parts rectangular, all placed, all holes in bounds).",🤖 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 `@skills/steel-nest/references/job_template.json` at line 29, Update the _comment_holes description in job_template.json to qualify burn-table DXF geometry by the burn_dxf_eligible gate, matching the behavior documented in SKILL.md. Keep the hole geometry, rotation, and weight/cost wording intact, but avoid promising HOLES-layer output when burn DXF generation is ineligible, such as for irregular parts.skills/steel-nest/SKILL.md-92-94 (1)
92-94: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the doctor script path
python3 scripts/nest.pyis skill-relative, sopython3 scripts/doctor.pypoints atskills/steel-nest/scripts/doctor.pyinstead of the repo-rootscripts/doctor.py. Use an explicit repo-root path or note the required working directory.🤖 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 `@skills/steel-nest/SKILL.md` around lines 92 - 94, Update the dependency and doctor-script instructions in SKILL.md to qualify the doctor.py location from the repository root, avoiding the skill-relative scripts/doctor.py resolution; use an explicit repo-root path or clearly specify the required working directory while preserving the existing optional formula-baking behavior.skills/_shared/pi_steel/__init__.py-26-47 (1)
26-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSort
__all__to satisfy Ruff RUF022.The active static analysis flags this export list as unsorted; alphabetize it so lint remains green.
🤖 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 `@skills/_shared/pi_steel/__init__.py` around lines 26 - 47, Sort the export names in __all__ alphabetically to satisfy Ruff RUF022, preserving every existing exported symbol and its spelling.Source: Linters/SAST tools
skills/_shared/pi_steel/cli.py-87-92 (1)
87-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
.parents[3]can raise an uncaughtIndexErrorin an error-handling helper.The index access happens outside the
try/except. Since this helper is invoked precisely while handling other failures (CLI usage errors, failure diagnostics), an out-of-range path would raise an unrelatedIndexErrorand mask the original error instead of degrading to"unknown".🛠️ Proposed fix
def package_version(entry_file: str) -> str: - package_path = Path(entry_file).resolve().parents[3] / "package.json" try: + package_path = Path(entry_file).resolve().parents[3] / "package.json" return json.loads(package_path.read_text(encoding="utf-8"))["version"] - except (OSError, KeyError, json.JSONDecodeError): + except (OSError, KeyError, IndexError, json.JSONDecodeError): return "unknown"🤖 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 `@skills/_shared/pi_steel/cli.py` around lines 87 - 92, Move the package_path construction inside the try block in package_version so an out-of-range Path.parents access is handled by the existing fallback. Preserve the current JSON loading behavior and return "unknown" for IndexError alongside the existing filesystem, key, and JSON errors.
🧹 Nitpick comments (17)
skills/_shared/schemas/nest-result.schema.json (1)
33-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
outcomeandrun_outcomeduplicate the same enum with no documented precedence.
outcomeis required,run_outcomeoptional, and both accept identical values. Consumers can't tell which wins if they disagree. Document the relationship in the schema (description) or drop the redundant field.🤖 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 `@skills/_shared/schemas/nest-result.schema.json` around lines 33 - 50, Clarify the relationship between the required outcome and optional run_outcome properties in the schema by adding descriptions that define their roles and precedence when values differ, or remove run_outcome if it is not independently necessary. Keep the enum values unchanged.skills/_shared/pi_steel/geometry_verify.py (1)
78-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnify the placement finiteness predicate.
Line 178-183 rejects
boolvalues, but_finite_placement_values(Line 79-83) accepts them, so aw: trueplacement is reported asnonfinite_placementyet still participates in overlap candidate generation. Share one predicate to keep the two paths consistent.♻️ Proposed refactor
+def _finite_number(value: Any) -> bool: + return ( + isinstance(value, (int, float)) + and not isinstance(value, bool) + and math.isfinite(value) + ) + + def _finite_placement_values(placement: dict[str, Any]) -> bool: - return all( - isinstance(placement.get(field), (int, float)) - and math.isfinite(placement[field]) - for field in ("x", "y", "w", "h") - ) + return all(_finite_number(placement.get(field)) for field in ("x", "y", "w", "h"))- if not all( - isinstance(value, (int, float)) - and not isinstance(value, bool) - and math.isfinite(value) - for value in values - ): + if not all(_finite_number(value) for value in values):Also applies to: 178-183
🤖 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 `@skills/_shared/pi_steel/geometry_verify.py` around lines 78 - 83, Unify placement validation by making _finite_placement_values use the same type predicate as the validation logic around the nonfinite_placement check, explicitly rejecting bool values while accepting finite numeric coordinates and dimensions. Reuse the shared predicate in both overlap candidate generation and validation so placements cannot pass one path and fail the other.skills/steel-nest/SKILL.md (1)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueArtifact list understates when
rfq_nesting.jsonis absent.
publish_nest_runwrites it only forready/review_required, so it is also missing fordependency_missingand internal-error runs, not just blocked ones.🤖 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 `@skills/steel-nest/SKILL.md` at line 80, Update the rfq_nesting.json artifact description in the artifact list to state that it is absent for dependency_missing and internal-error runs as well as blocked runs, while preserving that it is written for ready and review_required runs.skills/_shared/schemas/run-manifest.schema.json (1)
119-122: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConstrain
artifact.pathto a relative POSIX path.The publisher enforces "no absolute, no
.., not a reserved name", but the schema only requiresminLength: 1, so any consumer validating a third-party manifest gets no traversal protection. Mirror the runtime rule in the contract:🛡️ Proposed schema tightening
"path": { "type": "string", - "minLength": 1 + "minLength": 1, + "pattern": "^(?!/)(?!.*(^|/)\\.\\.(/|$))[^\\\\]+$" },Note JSON Schema
patternuses ECMA-262 regex; verify lookahead support in the validator in use.🤖 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 `@skills/_shared/schemas/run-manifest.schema.json` around lines 119 - 122, Update the artifact.path schema definition to enforce the publisher’s relative POSIX path rules: reject absolute paths, any .. traversal segment, and reserved path names while retaining the non-empty string constraint. Use an ECMA-262-compatible pattern supported by the project’s JSON Schema validator, and change only the path validation under the visible path property.skills/steel-nest/scripts/nest.py (2)
1118-1123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGroup utilization is re-derived from an already-rounded percentage.
pr["packing_utilization_pct"]["value"]is rounded to one decimal in_metric, sopacking_areahere is a lossy reconstruction and the groupedutilizationinherits that error. Carry the raw packed area (or raw ratio) on the plate report and aggregate that instead.🤖 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 `@skills/steel-nest/scripts/nest.py` around lines 1118 - 1123, Update the plate-report generation and group utilization aggregation around _metric to retain raw packed area or ratio data in each plate report; calculate packing_area from that unrounded value instead of pr["packing_utilization_pct"]["value"], while preserving the existing rounded output format for the final utilization.
1609-1616: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBurn artifacts are registered on
outcome == "ready"but written onburn_dxf_eligible.Today the two conditions are equivalent, so this works; if either predicate drifts,
render_burn_dxfsreturns[]while the paths are already registered andpublisher.publish()fails with an allow-list mismatch (or, in the reverse direction, files land unregistered). Gate registration on the same flag the renderer uses.♻️ Suggested tightening
- if outcome == "ready": + if outcome == "ready" and result["burn_dxf_eligible"]: for plate in result["plate_reports"]:🤖 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 `@skills/steel-nest/scripts/nest.py` around lines 1609 - 1616, Update the burn artifact registration block near render_burn_dxfs to use the same burn_dxf_eligible predicate that controls rendering, instead of checking outcome == "ready". Keep registration, rendering, and publishing aligned so only artifacts actually produced by render_burn_dxfs are registered.skills/_shared/pi_steel/run_manifest.py (1)
285-314: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
latest-run.jsonupdate is unsynchronized and non-durable.Two concurrent publishes can interleave between
os.replace(staging_path, final_path)and the pointer swap, leaving the pointer on the older run even though both directories exist. Also, neither the run directory nordestinationis fsynced after the rename, so a crash can leave a pointer that references a manifest whose bytes are not yet on disk. If concurrent stage invocations against one publication root are expected, consider an advisory lock file (or O_EXCL sentinel) around the rename + pointer swap, plusos.fsyncof the directory fd before returning.🤖 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 `@skills/_shared/pi_steel/run_manifest.py` around lines 285 - 314, Synchronize the publish flow surrounding self.final_path renaming and latest-run.json replacement with an advisory lock or O_EXCL sentinel scoped to the publication destination, so concurrent publishes cannot interleave. Hold the lock through rollback and pointer replacement, then fsync the published run directory and destination directory before returning to ensure the pointer references durable manifest bytes. Preserve the existing rollback and temporary-pointer cleanup behavior.requirements-dev.txt (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate Ruff pins in both requirements files
ruff>=0.11,<0.12andruff==0.11.13are both behind the current 0.16.0 release. Raise the ceiling and exact pin together so the dev and tested environments stay aligned.🤖 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 `@requirements-dev.txt` at line 5, Update Ruff in requirements-dev.txt at lines 5-5 to allow the current 0.16.0 release, and update the exact Ruff pin in requirements-tested.txt at lines 6-6 to 0.16.0 so both development and tested environments remain aligned.tests/fixtures/rfq/synthetic-profile.json (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument
tests/fixtures/rfq/synthetic-profile.jsonas the missing-logo case.
It already feeds RFQ tests that expect thetext_fallbacklogo path, so a short note intests/fixtures/rfq/README.mdwould make the scenario explicit.🤖 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 `@tests/fixtures/rfq/synthetic-profile.json` at line 4, Add a brief note to the RFQ fixtures README identifying synthetic-profile.json as the missing-optional-logo fixture and documenting that it exercises the text_fallback logo behavior; leave the fixture data unchanged.tests/test_data_provenance.py (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert on parsed JSON instead of formatted substring. Line 36 couples the test to the script's
indent=2formatting; parsing stdout is equally cheap and survives formatting changes.♻️ Parse the report
self.assertEqual(result.returncode, 2, result.stdout + result.stderr) - self.assertIn('"redistribution_permission": "unverified"', result.stdout) + report = json.loads(result.stdout) + self.assertEqual(report["redistribution_permission"], "unverified")(add
import json)🤖 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 `@tests/test_data_provenance.py` around lines 27 - 36, Update test_release_check_blocks_unverified_redistribution to import json and parse result.stdout as JSON, then assert the parsed report contains redistribution_permission equal to "unverified" instead of checking a formatted substring. Preserve the existing return-code assertion and failure diagnostics.tests/test_estimate_pipeline.py (2)
49-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailure diagnostics get masked here. If the CLI exits before publishing (import error, argparse failure), Line 73 raises
FileNotFoundErrorandcompleted.stdout/stderrare never shown, making failures hard to diagnose.♻️ Surface the CLI output when no run was published
- pointer = json.loads((output / "latest-run.json").read_text()) + pointer_path = output / "latest-run.json" + assert pointer_path.exists(), completed.stdout + completed.stderr + pointer = json.loads(pointer_path.read_text()) return completed, output / pointer["run_directory"]🤖 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 `@tests/test_estimate_pipeline.py` around lines 49 - 74, Update the test helper around the subprocess invocation and latest-run pointer loading to check whether the CLI completed successfully and published the expected output before reading latest-run.json. When no run is published, surface completed.stdout and completed.stderr in the failure instead of allowing an uninformative FileNotFoundError; preserve the existing pointer-based return path for successful runs.
253-272: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLine 272 is tautological.
blocked_pathis built from the same pointer read insiderun_pipeline, so this comparison always holds. Assert on the run id (or that the pointer no longer referencesready_path) to actually prove the pointer was replaced.🤖 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 `@tests/test_estimate_pipeline.py` around lines 253 - 272, The assertion comparing the published pointer-derived path to blocked_path in test_blocked_run_replaces_latest_pointer_without_reusing_ready_artifacts is tautological because run_pipeline already uses that pointer. Assert instead that pointer["run_directory"] identifies the blocked run or that the published pointer no longer references ready_path, while preserving the existing blocked-run and artifact assertions.tests/test_full_render_smoke.py (2)
176-182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicate words collapse silently in
word_positions. Keying by uppercase text keeps only the last occurrence'syMin, so if "TOTAL"/"TERMS"/"RESPONSE" appear more than once in the rendered sheet, Lines 180-182 assert ordering against an arbitrary occurrence (and raiseKeyErrorif a label is missing/split across words). Collect all occurrences and compare a well-defined extreme instead.♻️ Compare min/max positions per label
- word_positions = { - (word.text or "").upper(): float(word.attrib["yMin"]) for word in words - } - assert word_positions["REQUEST"] < page_height * 0.2 - assert word_positions["RESPONSE"] < word_positions["TOTAL"] - assert word_positions["TOTAL"] < word_positions["TERMS"] - assert word_positions["TERMS"] < page_height * 0.95 + positions: dict[str, list[float]] = {} + for word in words: + positions.setdefault((word.text or "").upper(), []).append( + float(word.attrib["yMin"]) + ) + for label in ("REQUEST", "RESPONSE", "TOTAL", "TERMS"): + assert positions.get(label), f"{label} missing from rendered PDF" + assert min(positions["REQUEST"]) < page_height * 0.2 + assert min(positions["RESPONSE"]) < min(positions["TOTAL"]) + assert min(positions["TOTAL"]) < max(positions["TERMS"]) + assert max(positions["TERMS"]) < page_height * 0.95🤖 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 `@tests/test_full_render_smoke.py` around lines 176 - 182, Update the word-position collection in the smoke test to retain every occurrence of each uppercase label instead of overwriting duplicates in a single-value dictionary. Use the collected positions to compare a defined extreme, such as each label’s minimum yMin, while preserving the existing ordering and page-boundary assertions for REQUEST, RESPONSE, TOTAL, and TERMS.
63-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnvironment isolation differs from the other pipeline harness.
tests/test_estimate_pipeline.pypopsPYTHONPATHand pinsXDG_CONFIG_HOMEbefore invoking the same script; here the full ambient environment is inherited, so a developer'sPYTHONPATH/user config can mask runtime bootstrap regressions in the render path.♻️ Match the isolation used elsewhere
environment = os.environ.copy() + environment.pop("PYTHONPATH", None) + environment["XDG_CONFIG_HOME"] = str(tmp_path / "empty-config") environment["PI_STEEL_CONFIG"] = str(profile_path)🤖 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 `@tests/test_full_render_smoke.py` around lines 63 - 65, Update the environment setup in the full-render smoke test around the environment variable preparation to match tests/test_estimate_pipeline.py: remove PYTHONPATH from the copied environment and pin XDG_CONFIG_HOME to the test-controlled configuration location before invoking the pipeline script. Preserve the existing PI_STEEL_CONFIG assignment and other test behavior.tests/test_acknowledge_finding_cli.py (1)
108-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name promises more than it asserts. It verifies the warning stays active but never checks the rejected acknowledgement was recorded in the output package.
♻️ Assert the recorded disposition
reviewed = json.loads(output.read_text(encoding="utf-8")) + assert reviewed["review"]["acknowledgements"][-1]["disposition"] == "rejected" assert any(🤖 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 `@tests/test_acknowledge_finding_cli.py` around lines 108 - 127, Update test_rejected_decision_is_recorded_without_clearing_warning to assert that the reviewed package records an acknowledgement for the target finding with disposition “rejected”, in addition to verifying the finding remains active.tests/test_contracts.py (2)
486-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder-stability assertion is vacuous.
forward == list(reversed(reverse))holds by construction of the two comprehensions for any pure function ofsource; it proves nothing about ordering independence. Consider asserting the ids against a set/dict built from a shuffled input instead.🤖 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 `@tests/test_contracts.py` around lines 486 - 503, Replace the vacuous forward/reverse list comparison in test_identifiers_are_reorder_stable_and_quantity_expansion_is_predictable with an order-independent assertion that maps each source to its generated identifier and compares results from shuffled or reordered input. Keep the existing revision-equivalence and quantity-expansion assertions unchanged.
544-559: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion doesn't verify the stated behavior. The test name claims the explicit cost "preserves basis", but it only asserts one code is absent — it would pass even if the whole package were rejected. Note the extra
"synthetic": Truekey (Line 555): ifcommercial_basis.costsentries are closed-typed likedimensions(Lines 163-166), this package is schema-invalid and the test still passes silently.♻️ Tighten the assertion
result = validate_estimate_package(package) - assert "invalid_cost_basis" not in blocker_codes(result) + assert "schema_validation" not in blocker_codes(result) + assert result.status == "review_required" + assert package["commercial_basis"]["costs"][0]["unit_basis"] == "per_pound"🤖 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 `@tests/test_contracts.py` around lines 544 - 559, Strengthen test_absent_costs_do_not_create_prices_and_explicit_cost_preserves_basis by asserting the package validates successfully, not merely that invalid_cost_basis is absent. Remove the synthetic field from the appended cost entry if cost entries are closed-typed, and assert the resulting validation status or blocker set demonstrates that the explicit per_pound basis is preserved.
🤖 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 @.gitignore:
- Around line 10-24: Add the outputs/ directory pattern to the local/private
generated-data ignore entries in .gitignore, alongside private/, local-data/,
and customer-data/, so generated output artifacts are excluded from Git.
In `@scripts/check-data-provenance.py`:
- Around line 54-64: Update the documented_values tuple in the provenance
consistency check to derive the redistribution-permission entry from the
existing permission variable instead of hardcoding “Unverified”. Preserve the
current validation of claimed_edition, expected_rows, and sha256 while ensuring
changes to redistribution_permission are detected.
- Around line 19-36: Update the dataset validation flow after checking
len(datasets) in the provenance-checking function: when the record does not
contain exactly one dataset, report the existing field-specific error and stop
before constructing shapes_path or reading bytes. Preserve normal checksum and
row validation when exactly one dataset is present, and ensure malformed records
return the collected diagnostics rather than raising a filesystem exception.
In `@scripts/check-public-data.py`:
- Line 41: Correct the “local absolute path” regex in the public-data checks to
match single backslashes in normal Windows paths such as C:\Users\alice, while
preserving the existing Unix path matching.
In `@skills/_shared/pi_steel/contracts.py`:
- Around line 69-79: Update estimate_input_projection to remove
reviewer_confirmation only when stock is a list and each entry is an object;
otherwise preserve the malformed JSON value unchanged so estimate_input_hash
remains computable and schema validation can report the schema_validation
blocker.
In `@skills/_shared/pi_steel/parsing.py`:
- Around line 18-25: Update parse_length_ft and the CSV field parsing around the
quantity, length, and unit-weight handling to tolerate blank or non-numeric
cells instead of raising ValueError. Preserve missing or invalid values as None
or raw text so validation.py can emit invalid_quantity, invalid_member_length,
and invalid_unit_weight findings; ensure present-but-empty cells receive the
same treatment as missing values.
- Around line 79-105: The legacy BOM adaptation in adapt_legacy_bom_csv adds
fabricated-part-only fields to every intent, causing schema rejection for
non-fabricated variants. In skills/_shared/pi_steel/parsing.py lines 79-105,
gate designation, length_ft, unit_weight_plf, connections, notes, and
total_weight_lbs on intent == fabricated_part; place identity_warning on the
shared itemBase-compatible structure if it must apply across intents. In
skills/_shared/schemas/estimate-package.schema.json lines 166-222, verify the
resulting placement matches the shared schema and requires no non-fabricated
variant properties.
- Around line 179-196: The legacy stock mapping in the parsing flow should
preserve unlimited inventory instead of converting it to quantity 1. Update the
stock objects created in the legacy stock loop to carry the unlimited flag
through the canonical stock model, while retaining existing quantity behavior
for finite stock and using the established canonical field name.
In `@skills/_shared/pi_steel/validation.py`:
- Around line 107-112: Update the package dependency configuration for
jsonschema to include its format extra, or explicitly add rfc3339-validator.
Keep _schema_validator using jsonschema.FormatChecker() so date and date-time
fields such as estimate_as_of, measured_at, and timestamp reject invalid values.
In `@skills/steel-estimate/scripts/build-estimate-package.py`:
- Around line 89-102: Update the findings and acknowledgements sort keys in
normalized_package to handle non-dict entries safely, matching the defensive
behavior used for items and stock. Ensure malformed entries such as strings do
not raise AttributeError, allowing validation-blocked packages to continue
producing their normal blocked-run or failure diagnostic.
- Around line 464-473: Update the profile blocking decision near
profile_findings so only findings whose severity is “error” set profile_blocked
to true; warning-level findings must remain non-blocking and follow the
documented review_required output path while preserving their warning severity
in findings.
- Around line 809-830: Extend the top-level exception handling around
build_pipeline in the main execution flow to catch RfqInputError and
ManifestError alongside the existing failure types. Ensure these errors use the
same publish_failure_diagnostic and stderr reporting path, preserving the
current return code and diagnostic-publication fallback behavior.
In `@skills/steel-nest/scripts/nest.py`:
- Around line 858-871: Update the per-part aggregation around parts_on and
part_net_cost to key entries by each placement’s item_id instead of
placement.label, preserving separate buckets for distinct normalized items.
Carry the corresponding display label alongside each keyed entry, then update
render_text and affected golden fixtures to display labels while retaining
item_id-based aggregation and separate rows for matching names.
- Around line 1648-1667: Add ManifestError to the pi_steel imports and include
it in the exception tuple around publish_nest_run in the main failure handler,
so manifest-related publication failures use publish_failure_diagnostic, emit
the controlled error message, and return exit code 1 instead of escaping as
tracebacks.
In `@skills/steel-rfq/scripts/generate-rfq.py`:
- Around line 406-415: Validate the result of json.loads in the profile-loading
loop before assigning _profile_path, and raise RfqInputError for arrays or
scalar JSON values. Keep valid object profiles returning with their source
unchanged so publish_rfq_run and main continue handling malformed profiles
through their existing diagnostic paths.
- Around line 339-357: Normalize and validate legacy Thickness in the workbook
adapter before constructing each item, rejecting free-text or otherwise
non-numeric values rather than passing them through. Ensure accepted values use
one consistent numeric representation so the item sort and compile_workbook
grouping sort can compare them with the -1 fallback without TypeError, while
preserving the adapter’s no-guessing contract.
- Around line 1197-1215: Extend the structured exception handling around
publish_rfq_run in the main generation flow to catch RecalculationError
alongside the existing OSError, JSON decode, and input errors. Ensure this
exception follows the same publish_failure_diagnostic, stderr reporting, and
exit-1 path, preserving the documented failure behavior.
In `@tests/test_installed_scripts.py`:
- Line 25: Update the test around package.extractall to remain compatible with
Python 3.11: use the filter argument only on Python 3.12 and newer, while using
the existing compatible extraction path on older supported interpreters.
Preserve the test’s extraction behavior across the supported Python range.
---
Minor comments:
In `@skills/_shared/pi_steel/__init__.py`:
- Around line 26-47: Sort the export names in __all__ alphabetically to satisfy
Ruff RUF022, preserving every existing exported symbol and its spelling.
In `@skills/_shared/pi_steel/cli.py`:
- Around line 87-92: Move the package_path construction inside the try block in
package_version so an out-of-range Path.parents access is handled by the
existing fallback. Preserve the current JSON loading behavior and return
"unknown" for IndexError alongside the existing filesystem, key, and JSON
errors.
In `@skills/_shared/pi_steel/validation.py`:
- Around line 447-471: Align the on-hand stock validation near the confirmation
predicate with the eligible_on_hand_stock filtering logic so reserved stock is
not silently ignored. Prefer preserving reserved as valid while adding a
reserved_on_hand_stock warning for confirmed reserved entries, and ensure the
warning points to the stock item without changing available-stock consumption
behavior.
In `@skills/steel-estimate/scripts/acknowledge-finding.py`:
- Around line 136-146: Wrap the os.link call in the acknowledgement-writing flow
with handling that converts FileExistsError into AcknowledgementInputError,
preserving the original failure context so main’s existing handler emits the
standard “Acknowledgement failed:” message. Keep the temporary-file cleanup in
the finally block unchanged.
In `@skills/steel-nest/references/job_template.json`:
- Line 29: Update the _comment_holes description in job_template.json to qualify
burn-table DXF geometry by the burn_dxf_eligible gate, matching the behavior
documented in SKILL.md. Keep the hole geometry, rotation, and weight/cost
wording intact, but avoid promising HOLES-layer output when burn DXF generation
is ineligible, such as for irregular parts.
In `@skills/steel-nest/SKILL.md`:
- Around line 92-94: Update the dependency and doctor-script instructions in
SKILL.md to qualify the doctor.py location from the repository root, avoiding
the skill-relative scripts/doctor.py resolution; use an explicit repo-root path
or clearly specify the required working directory while preserving the existing
optional formula-baking behavior.
In `@skills/steel-rfq/references/rfq-input.md`:
- Around line 22-34: Update the versioned rfq_nesting.json example to include
project_id, revision_id, and estimate_input_hash fields required by
validate_nest_handoff. Add a concise note stating these identity values must
match the estimate package or the handoff is rejected as stale, alongside the
existing version and boundary rules.
In `@skills/steel-takeoff/scripts/calculate-weight.sh`:
- Line 7: Update the usage/help text in calculate-weight.sh to include Source_ID
as the first BOM column, matching the documented CSV header and ensuring
generated CSV files are compatible.
---
Nitpick comments:
In `@requirements-dev.txt`:
- Line 5: Update Ruff in requirements-dev.txt at lines 5-5 to allow the current
0.16.0 release, and update the exact Ruff pin in requirements-tested.txt at
lines 6-6 to 0.16.0 so both development and tested environments remain aligned.
In `@skills/_shared/pi_steel/geometry_verify.py`:
- Around line 78-83: Unify placement validation by making
_finite_placement_values use the same type predicate as the validation logic
around the nonfinite_placement check, explicitly rejecting bool values while
accepting finite numeric coordinates and dimensions. Reuse the shared predicate
in both overlap candidate generation and validation so placements cannot pass
one path and fail the other.
In `@skills/_shared/pi_steel/run_manifest.py`:
- Around line 285-314: Synchronize the publish flow surrounding self.final_path
renaming and latest-run.json replacement with an advisory lock or O_EXCL
sentinel scoped to the publication destination, so concurrent publishes cannot
interleave. Hold the lock through rollback and pointer replacement, then fsync
the published run directory and destination directory before returning to ensure
the pointer references durable manifest bytes. Preserve the existing rollback
and temporary-pointer cleanup behavior.
In `@skills/_shared/schemas/nest-result.schema.json`:
- Around line 33-50: Clarify the relationship between the required outcome and
optional run_outcome properties in the schema by adding descriptions that define
their roles and precedence when values differ, or remove run_outcome if it is
not independently necessary. Keep the enum values unchanged.
In `@skills/_shared/schemas/run-manifest.schema.json`:
- Around line 119-122: Update the artifact.path schema definition to enforce the
publisher’s relative POSIX path rules: reject absolute paths, any .. traversal
segment, and reserved path names while retaining the non-empty string
constraint. Use an ECMA-262-compatible pattern supported by the project’s JSON
Schema validator, and change only the path validation under the visible path
property.
In `@skills/steel-nest/scripts/nest.py`:
- Around line 1118-1123: Update the plate-report generation and group
utilization aggregation around _metric to retain raw packed area or ratio data
in each plate report; calculate packing_area from that unrounded value instead
of pr["packing_utilization_pct"]["value"], while preserving the existing rounded
output format for the final utilization.
- Around line 1609-1616: Update the burn artifact registration block near
render_burn_dxfs to use the same burn_dxf_eligible predicate that controls
rendering, instead of checking outcome == "ready". Keep registration, rendering,
and publishing aligned so only artifacts actually produced by render_burn_dxfs
are registered.
In `@skills/steel-nest/SKILL.md`:
- Line 80: Update the rfq_nesting.json artifact description in the artifact list
to state that it is absent for dependency_missing and internal-error runs as
well as blocked runs, while preserving that it is written for ready and
review_required runs.
In `@tests/fixtures/rfq/synthetic-profile.json`:
- Line 4: Add a brief note to the RFQ fixtures README identifying
synthetic-profile.json as the missing-optional-logo fixture and documenting that
it exercises the text_fallback logo behavior; leave the fixture data unchanged.
In `@tests/test_acknowledge_finding_cli.py`:
- Around line 108-127: Update
test_rejected_decision_is_recorded_without_clearing_warning to assert that the
reviewed package records an acknowledgement for the target finding with
disposition “rejected”, in addition to verifying the finding remains active.
In `@tests/test_contracts.py`:
- Around line 486-503: Replace the vacuous forward/reverse list comparison in
test_identifiers_are_reorder_stable_and_quantity_expansion_is_predictable with
an order-independent assertion that maps each source to its generated identifier
and compares results from shuffled or reordered input. Keep the existing
revision-equivalence and quantity-expansion assertions unchanged.
- Around line 544-559: Strengthen
test_absent_costs_do_not_create_prices_and_explicit_cost_preserves_basis by
asserting the package validates successfully, not merely that invalid_cost_basis
is absent. Remove the synthetic field from the appended cost entry if cost
entries are closed-typed, and assert the resulting validation status or blocker
set demonstrates that the explicit per_pound basis is preserved.
In `@tests/test_data_provenance.py`:
- Around line 27-36: Update test_release_check_blocks_unverified_redistribution
to import json and parse result.stdout as JSON, then assert the parsed report
contains redistribution_permission equal to "unverified" instead of checking a
formatted substring. Preserve the existing return-code assertion and failure
diagnostics.
In `@tests/test_estimate_pipeline.py`:
- Around line 49-74: Update the test helper around the subprocess invocation and
latest-run pointer loading to check whether the CLI completed successfully and
published the expected output before reading latest-run.json. When no run is
published, surface completed.stdout and completed.stderr in the failure instead
of allowing an uninformative FileNotFoundError; preserve the existing
pointer-based return path for successful runs.
- Around line 253-272: The assertion comparing the published pointer-derived
path to blocked_path in
test_blocked_run_replaces_latest_pointer_without_reusing_ready_artifacts is
tautological because run_pipeline already uses that pointer. Assert instead that
pointer["run_directory"] identifies the blocked run or that the published
pointer no longer references ready_path, while preserving the existing
blocked-run and artifact assertions.
In `@tests/test_full_render_smoke.py`:
- Around line 176-182: Update the word-position collection in the smoke test to
retain every occurrence of each uppercase label instead of overwriting
duplicates in a single-value dictionary. Use the collected positions to compare
a defined extreme, such as each label’s minimum yMin, while preserving the
existing ordering and page-boundary assertions for REQUEST, RESPONSE, TOTAL, and
TERMS.
- Around line 63-65: Update the environment setup in the full-render smoke test
around the environment variable preparation to match
tests/test_estimate_pipeline.py: remove PYTHONPATH from the copied environment
and pin XDG_CONFIG_HOME to the test-controlled configuration location before
invoking the pipeline script. Preserve the existing PI_STEEL_CONFIG assignment
and other test behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf9c2878-c70c-4c6b-ad0d-afc2dfb0feac
⛔ Files ignored due to path filters (4)
skills/steel-takeoff/assets/bom-template.csvis excluded by!**/*.csvtests/fixtures/contracts/legacy-bom.csvis excluded by!**/*.csvtests/fixtures/workflows/ambiguous_scope.csvis excluded by!**/*.csvtests/fixtures/workflows/member_only.csvis excluded by!**/*.csv
📒 Files selected for processing (85)
.github/workflows/ci.yml.gitignore.npmignoreDATA_PROVENANCE.jsonDATA_PROVENANCE.mdPUBLIC_DATA_POLICY.mdREADME.mddocs/plans/2026-07-28-001-feat-trustworthy-estimate-package-plan.mddocs/research/v0.3-workflow-evidence.mdpackage.jsonpyproject.tomlrequirements-dev.txtrequirements-render.txtrequirements-tested.txtrequirements.txtscripts/check-data-provenance.pyscripts/check-public-data.pyscripts/doctor.pyskills/_shared/bootstrap.pyskills/_shared/pi_steel/__init__.pyskills/_shared/pi_steel/cli.pyskills/_shared/pi_steel/contracts.pyskills/_shared/pi_steel/geometry_verify.pyskills/_shared/pi_steel/parsing.pyskills/_shared/pi_steel/run_manifest.pyskills/_shared/pi_steel/validation.pyskills/_shared/schemas/estimate-package.schema.jsonskills/_shared/schemas/nest-result.schema.jsonskills/_shared/schemas/run-manifest.schema.jsonskills/steel-estimate/SKILL.mdskills/steel-estimate/references/estimate-package-example.jsonskills/steel-estimate/references/output-contract.mdskills/steel-estimate/scripts/acknowledge-finding.pyskills/steel-estimate/scripts/build-estimate-package.pyskills/steel-nest/SKILL.mdskills/steel-nest/references/FIXTURE_PROVENANCE.mdskills/steel-nest/references/example_job.jsonskills/steel-nest/references/job_template.jsonskills/steel-nest/scripts/nest.pyskills/steel-rfq/SKILL.mdskills/steel-rfq/assets/company-profile.example.jsonskills/steel-rfq/references/rfq-input.mdskills/steel-rfq/scripts/generate-rfq.pyskills/steel-rfq/scripts/recalc.pyskills/steel-takeoff/SKILL.mdskills/steel-takeoff/references/takeoff-procedures.mdskills/steel-takeoff/scripts/calculate-weight.shskills/steel-takeoff/scripts/validate-bom.pytests/fixtures/contracts/README.mdtests/fixtures/contracts/legacy-nest.jsontests/fixtures/nest/README.mdtests/fixtures/nest/grouped-engine.jsontests/fixtures/nest/irregular-reference-only.jsontests/fixtures/pipeline/README.mdtests/fixtures/pipeline/synthetic-estimate.jsontests/fixtures/pipeline/synthetic-profile.jsontests/fixtures/rfq/README.mdtests/fixtures/rfq/estimate-package.jsontests/fixtures/rfq/nest-handoff.jsontests/fixtures/rfq/synthetic-profile.jsontests/fixtures/workflows/README.mdtests/fixtures/workflows/blocked_plate_job.jsontests/fixtures/workflows/mixed_plate_job.jsontests/golden/pipeline/ready-artifacts.jsontests/golden/rfq/semantic-workbook.jsontests/test_acknowledge_finding_cli.pytests/test_contracts.pytests/test_data_provenance.pytests/test_estimate_pipeline.pytests/test_full_render_smoke.pytests/test_installed_scripts.pytests/test_nest_burn_guard.pytests/test_nest_cli_contract.pytests/test_nest_engine.pytests/test_nest_invariants.pytests/test_nest_outputs.pytests/test_package_contents.pytests/test_public_data_policy.pytests/test_recalc.pytests/test_rfq_generator.pytests/test_rfq_workbook_contract.pytests/test_run_manifests.pytests/test_runtime_bootstrap.pytests/test_structured_failure_cli.pytests/test_takeoff_cli.py
| provenance = PROVENANCE_PATH.read_text(encoding="utf-8") | ||
| permission = dataset.get("redistribution_permission") | ||
| release_readiness = dataset.get("release_readiness") | ||
| documented_values = ( | ||
| dataset.get("claimed_edition"), | ||
| str(expected_rows), | ||
| dataset.get("sha256"), | ||
| "Redistribution permission | Unverified", | ||
| ) | ||
| if not all(value and value in provenance for value in documented_values): | ||
| errors.append("DATA_PROVENANCE.md differs from the machine-readable record") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Consistency check ignores the permission value it reads.
permission is read from the record but the documentation cross-check hardcodes the literal string "Redistribution permission | Unverified" instead of interpolating permission. The check currently only passes because the live value happens to be "Unverified"; it will silently stop validating (or wrongly fail) once redistribution_permission changes, which the plan explicitly calls out as an upcoming release-viability gate.
🛠️ Proposed fix
documented_values = (
dataset.get("claimed_edition"),
str(expected_rows),
dataset.get("sha256"),
- "Redistribution permission | Unverified",
+ f"Redistribution permission | {permission}",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| provenance = PROVENANCE_PATH.read_text(encoding="utf-8") | |
| permission = dataset.get("redistribution_permission") | |
| release_readiness = dataset.get("release_readiness") | |
| documented_values = ( | |
| dataset.get("claimed_edition"), | |
| str(expected_rows), | |
| dataset.get("sha256"), | |
| "Redistribution permission | Unverified", | |
| ) | |
| if not all(value and value in provenance for value in documented_values): | |
| errors.append("DATA_PROVENANCE.md differs from the machine-readable record") | |
| provenance = PROVENANCE_PATH.read_text(encoding="utf-8") | |
| permission = dataset.get("redistribution_permission") | |
| release_readiness = dataset.get("release_readiness") | |
| documented_values = ( | |
| dataset.get("claimed_edition"), | |
| str(expected_rows), | |
| dataset.get("sha256"), | |
| f"Redistribution permission | {permission}", | |
| ) | |
| if not all(value and value in provenance for value in documented_values): | |
| errors.append("DATA_PROVENANCE.md differs from the machine-readable record") |
🤖 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-data-provenance.py` around lines 54 - 64, Update the
documented_values tuple in the provenance consistency check to derive the
redistribution-permission entry from the existing permission variable instead of
hardcoding “Unverified”. Preserve the current validation of claimed_edition,
expected_rows, and sha256 while ensuring changes to redistribution_permission
are detected.
| "private operating-company claim": re.compile( | ||
| r"team behind (?:a|the) production structural[- ]steel", re.I | ||
| ), | ||
| "local absolute path": re.compile(r"(?:/Users/|/home/|[A-Z]:\\\\Users\\\\)"), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does re.compile(r"[A-Z]:\\\\Users\\\\") match a string containing a single backslash "C:\\Users\\"?
💡 Result:
The regular expression pattern r"[A-Z]:\\Users\\" will not match a string containing a single backslash like C:\Users. [1][2] In Python regular expressions, the backslash character is an escape character used to signal special sequences or to escape metacharacters [1][2]. To match a literal backslash in a regular expression, you must escape it with another backslash within the regex pattern itself [1]. Because you are using a raw string literal (r""), the pattern is passed to the regex engine as [A-Z]:\\Users\\. 1. Within the regular expression language, a double backslash (\) represents a single literal backslash character [1][2]. 2. Your pattern r"[A-Z]:\\Users\\" contains two pairs of double backslashes: - The first pair matches a literal backslash after the colon [1][2]. - The second pair matches a literal backslash at the end [1][2]. Consequently, the regex expects the format C:\Users\ (with two literal backslashes). Since your target string C:\Users\ only contains a single literal backslash at those positions, the match will fail [1][3]. To match a string with a single literal backslash at those positions, you should use a pattern that expects only one literal backslash, such as r"[A-Z]:\Users\" [1][4].
Citations:
- 1: https://docs.python.org/3/howto/regex.html
- 2: https://stackoverflow.com/questions/30172635/python-regex-not-able-to-match-strings-with-back-slash-involved
- 3: https://stackoverflow.com/questions/51541228/how-is-python-regex-evaluated
- 4: https://stackoverflow.com/questions/48972155/how-to-quote-backslash-in-python-code-four-to-quote-one
🏁 Script executed:
python3 - <<'PY'
import re
pattern = re.compile(r"(?:/Users/|/home/|[A-Z]:\\\\Users\\\\)")
samples = [
r"C:\Users\alice",
r"C:\\Users\\alice",
r"C:\Users\",
r"C:\\Users\\",
]
for s in samples:
print(repr(s), bool(pattern.search(s)))
PYRepository: StructuPath/pi-steel
Length of output: 267
🏁 Script executed:
python3 - <<'PY'
import re
pattern = re.compile(r"(?:/Users/|/home/|[A-Z]:\\\\Users\\\\)")
samples = {
"single-backslash path": "C:\\Users\\alice",
"double-backslash text": "C:\\\\Users\\\\alice",
"single-backslash root": "C:\\Users\\",
"double-backslash root text": "C:\\\\Users\\\\",
}
for label, s in samples.items():
m = pattern.search(s)
print(label, repr(s), bool(m), m.group(0) if m else None)
PYRepository: StructuPath/pi-steel
Length of output: 381
Windows absolute-path pattern is over-escaped
r"(?:/Users/|/home/|[A-Z]:\\\\Users\\\\)" only matches doubled-backslash text, so a normal Windows path like C:\Users\alice slips through. Use r"(?:/Users/|/home/|[A-Z]:\\Users\\)" instead.
🤖 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-public-data.py` at line 41, Correct the “local absolute path”
regex in the public-data checks to match single backslashes in normal Windows
paths such as C:\Users\alice, while preserving the existing Unix path matching.
| def estimate_input_projection(package: dict[str, Any]) -> dict[str, Any]: | ||
| """Return source/config semantics, excluding review and confirmation bookkeeping.""" | ||
| projection = { | ||
| key: value | ||
| for key, value in package.items() | ||
| if key not in {"review"} | ||
| } | ||
| projection = json.loads(json.dumps(projection)) | ||
| for stock in projection.get("stock", []): | ||
| stock.pop("reviewer_confirmation", None) | ||
| return projection |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Malformed stock crashes hashing before schema validation can report it.
validate_estimate_package computes estimate_input_hash(package) (validation.py Line 299) before any schema check. If stock is not a list of objects (e.g. "stock": ["oops"] or a dict), stock.pop(...) raises AttributeError, so an invalid package aborts the CLI with a traceback instead of producing a schema_validation blocker finding. Guard the shape here so the identity hash is computable for any JSON input.
🛡️ Proposed fix
projection = json.loads(json.dumps(projection))
- for stock in projection.get("stock", []):
- stock.pop("reviewer_confirmation", None)
+ stock_entries = projection.get("stock")
+ if isinstance(stock_entries, list):
+ for stock in stock_entries:
+ if isinstance(stock, dict):
+ stock.pop("reviewer_confirmation", None)
return projection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def estimate_input_projection(package: dict[str, Any]) -> dict[str, Any]: | |
| """Return source/config semantics, excluding review and confirmation bookkeeping.""" | |
| projection = { | |
| key: value | |
| for key, value in package.items() | |
| if key not in {"review"} | |
| } | |
| projection = json.loads(json.dumps(projection)) | |
| for stock in projection.get("stock", []): | |
| stock.pop("reviewer_confirmation", None) | |
| return projection | |
| def estimate_input_projection(package: dict[str, Any]) -> dict[str, Any]: | |
| """Return source/config semantics, excluding review and confirmation bookkeeping.""" | |
| projection = { | |
| key: value | |
| for key, value in package.items() | |
| if key not in {"review"} | |
| } | |
| projection = json.loads(json.dumps(projection)) | |
| stock_entries = projection.get("stock") | |
| if isinstance(stock_entries, list): | |
| for stock in stock_entries: | |
| if isinstance(stock, dict): | |
| stock.pop("reviewer_confirmation", None) | |
| return projection |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 75-75: use jsonify instead of json.dumps for JSON output
Context: json.dumps(projection)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@skills/_shared/pi_steel/contracts.py` around lines 69 - 79, Update
estimate_input_projection to remove reviewer_confirmation only when stock is a
list and each entry is an object; otherwise preserve the malformed JSON value
unchanged so estimate_input_hash remains computable and schema validation can
report the schema_validation blocker.
| try: | ||
| with open(args.job, encoding="utf-8") as f: | ||
| job = json.load(f) | ||
| result, qa_report, report, final_path = publish_nest_run(job, args) | ||
| except (OSError, json.JSONDecodeError, TypeError, ValueError) as exc: | ||
| diagnostic_path = publish_failure_diagnostic( | ||
| args.out, | ||
| stage="steel-nest", | ||
| input_path=args.job, | ||
| error=exc, | ||
| tool_version=package_version(__file__), | ||
| run_id=args.run_id, | ||
| ) | ||
| suffix = ( | ||
| f"; diagnostic published: {diagnostic_path}" | ||
| if diagnostic_path is not None | ||
| else "; diagnostic publication unavailable" | ||
| ) | ||
| print(f"Nesting failed: {exc}{suffix}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ManifestError escapes the structured failure path.
ManifestError subclasses RuntimeError, so publication failures raised inside publish_nest_run (run already exists, artifact allow-list mismatch, latest-run pointer update failed) bypass this handler and surface as a traceback with no published diagnostic and no controlled exit code.
🐛 Proposed fix
- except (OSError, json.JSONDecodeError, TypeError, ValueError) as exc:
+ except (
+ OSError,
+ json.JSONDecodeError,
+ TypeError,
+ ValueError,
+ ManifestError,
+ ) as exc:with ManifestError added to the pi_steel import block at lines 55-66.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1648-1648: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(args.job, encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@skills/steel-nest/scripts/nest.py` around lines 1648 - 1667, Add
ManifestError to the pi_steel imports and include it in the exception tuple
around publish_nest_run in the main failure handler, so manifest-related
publication failures use publish_failure_diagnostic, emit the controlled error
message, and return exit code 1 instead of escaping as tracebacks.
| "thickness": row["Thickness"], | ||
| "quantity": quantity, | ||
| "stock_length": "", | ||
| "purchase_weight_lbs": row["Purchase Weight"], | ||
| "replaces_item_ids": (), | ||
| } | ||
| items.append(item) | ||
| if len(currencies) != 1: | ||
| raise RfqInputError("legacy workbook must use one explicit currency") | ||
| items.sort( | ||
| key=lambda item: ( | ||
| CATEGORY_ORDER.get(item["category"], 80), | ||
| item["material"], | ||
| item["grade"], | ||
| item["thickness"] if item["thickness"] is not None else -1, | ||
| item["size"], | ||
| item["item_id"], | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Legacy Thickness is passed through untyped, so the sort can raise TypeError.
row["Thickness"] keeps whatever the cell held (float, int, or free text such as 1/2"). The sort key at Lines 348-357 mixes those with the -1 fallback, and str vs float comparison raises TypeError; the same values later feed the grouping sort in compile_workbook (Lines 762-771). Reject or coerce non-numeric thickness in the adapter, consistent with its "no guessing" contract.
🛠️ Proposed fix
+ thickness = row["Thickness"]
+ if thickness is not None and not isinstance(thickness, (int, float)):
+ raise RfqInputError(
+ f"legacy row {row_number} has non-numeric Thickness"
+ )
@@
- "thickness": row["Thickness"],
+ "thickness": thickness,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "thickness": row["Thickness"], | |
| "quantity": quantity, | |
| "stock_length": "", | |
| "purchase_weight_lbs": row["Purchase Weight"], | |
| "replaces_item_ids": (), | |
| } | |
| items.append(item) | |
| if len(currencies) != 1: | |
| raise RfqInputError("legacy workbook must use one explicit currency") | |
| items.sort( | |
| key=lambda item: ( | |
| CATEGORY_ORDER.get(item["category"], 80), | |
| item["material"], | |
| item["grade"], | |
| item["thickness"] if item["thickness"] is not None else -1, | |
| item["size"], | |
| item["item_id"], | |
| ) | |
| ) | |
| thickness = row["Thickness"] | |
| if thickness is not None and not isinstance(thickness, (int, float)): | |
| raise RfqInputError( | |
| f"legacy row {row_number} has non-numeric Thickness" | |
| ) | |
| item = { | |
| "thickness": thickness, | |
| "quantity": quantity, | |
| "stock_length": "", | |
| "purchase_weight_lbs": row["Purchase Weight"], | |
| "replaces_item_ids": (), | |
| } | |
| items.append(item) | |
| if len(currencies) != 1: | |
| raise RfqInputError("legacy workbook must use one explicit currency") | |
| items.sort( | |
| key=lambda item: ( | |
| CATEGORY_ORDER.get(item["category"], 80), | |
| item["material"], | |
| item["grade"], | |
| item["thickness"] if item["thickness"] is not None else -1, | |
| item["size"], | |
| item["item_id"], | |
| ) | |
| ) |
🤖 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 `@skills/steel-rfq/scripts/generate-rfq.py` around lines 339 - 357, Normalize
and validate legacy Thickness in the workbook adapter before constructing each
item, rejecting free-text or otherwise non-numeric values rather than passing
them through. Ensure accepted values use one consistent numeric representation
so the item sort and compile_workbook grouping sort can compare them with the -1
fallback without TypeError, while preserving the adapter’s no-guessing contract.
| for path, source in candidates: | ||
| if not path.is_file(): | ||
| continue | ||
| try: | ||
| profile = json.loads(path.read_text(encoding="utf-8")) | ||
| except (OSError, json.JSONDecodeError) as exc: | ||
| raise RfqInputError(f"company profile could not be read: {exc}") from exc | ||
| profile["_profile_path"] = str(path) | ||
| return profile, source | ||
| return None, "missing" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-object profile JSON raises an unhandled TypeError.
A profile file containing a JSON array or scalar passes json.loads, then profile["_profile_path"] = ... raises TypeError, which is not an RfqInputError and so bypasses both the invalid_company_profile finding in publish_rfq_run and main's diagnostic handler.
🛠️ Proposed fix
except (OSError, json.JSONDecodeError) as exc:
raise RfqInputError(f"company profile could not be read: {exc}") from exc
+ if not isinstance(profile, dict):
+ raise RfqInputError(
+ f"company profile must be a JSON object: {path}"
+ )
profile["_profile_path"] = str(path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for path, source in candidates: | |
| if not path.is_file(): | |
| continue | |
| try: | |
| profile = json.loads(path.read_text(encoding="utf-8")) | |
| except (OSError, json.JSONDecodeError) as exc: | |
| raise RfqInputError(f"company profile could not be read: {exc}") from exc | |
| profile["_profile_path"] = str(path) | |
| return profile, source | |
| return None, "missing" | |
| for path, source in candidates: | |
| if not path.is_file(): | |
| continue | |
| try: | |
| profile = json.loads(path.read_text(encoding="utf-8")) | |
| except (OSError, json.JSONDecodeError) as exc: | |
| raise RfqInputError(f"company profile could not be read: {exc}") from exc | |
| if not isinstance(profile, dict): | |
| raise RfqInputError( | |
| f"company profile must be a JSON object: {path}" | |
| ) | |
| profile["_profile_path"] = str(path) | |
| return profile, source | |
| return None, "missing" |
🤖 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 `@skills/steel-rfq/scripts/generate-rfq.py` around lines 406 - 415, Validate
the result of json.loads in the profile-loading loop before assigning
_profile_path, and raise RfqInputError for arrays or scalar JSON values. Keep
valid object profiles returning with their source unchanged so publish_rfq_run
and main continue handling malformed profiles through their existing diagnostic
paths.
| try: | ||
| qa_report, final_path = publish_rfq_run(args) | ||
| except (OSError, json.JSONDecodeError, RfqInputError) as exc: | ||
| diagnostic_path = publish_failure_diagnostic( | ||
| args.out, | ||
| stage="steel-rfq", | ||
| input_path=args.input, | ||
| error=exc, | ||
| tool_version=package_version(__file__), | ||
| run_id=args.run_id, | ||
| explicit_dates={"issued_date": args.issued_date}, | ||
| ) | ||
| suffix = ( | ||
| f"; diagnostic published: {diagnostic_path}" | ||
| if diagnostic_path is not None | ||
| else "; diagnostic publication unavailable" | ||
| ) | ||
| print(f"RFQ generation failed: {exc}{suffix}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
RecalculationError escapes the structured failure path.
recalc.recalculate (called at Line 994) raises RecalculationError(RuntimeError), which is not in the caught tuple. A workbook that cannot be re-saved with recalc flags therefore aborts with a raw traceback, leaving the staging run unpublished and no failure diagnostic — while the documented contract (skills/steel-rfq/SKILL.md Lines 76-81) promises exit 1 plus a diagnostic for internal errors.
🛠️ Proposed fix
try:
qa_report, final_path = publish_rfq_run(args)
- except (OSError, json.JSONDecodeError, RfqInputError) as exc:
+ except (
+ OSError,
+ json.JSONDecodeError,
+ RfqInputError,
+ recalc.RecalculationError,
+ ) as exc:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| qa_report, final_path = publish_rfq_run(args) | |
| except (OSError, json.JSONDecodeError, RfqInputError) as exc: | |
| diagnostic_path = publish_failure_diagnostic( | |
| args.out, | |
| stage="steel-rfq", | |
| input_path=args.input, | |
| error=exc, | |
| tool_version=package_version(__file__), | |
| run_id=args.run_id, | |
| explicit_dates={"issued_date": args.issued_date}, | |
| ) | |
| suffix = ( | |
| f"; diagnostic published: {diagnostic_path}" | |
| if diagnostic_path is not None | |
| else "; diagnostic publication unavailable" | |
| ) | |
| print(f"RFQ generation failed: {exc}{suffix}", file=sys.stderr) | |
| return 1 | |
| try: | |
| qa_report, final_path = publish_rfq_run(args) | |
| except ( | |
| OSError, | |
| json.JSONDecodeError, | |
| RfqInputError, | |
| recalc.RecalculationError, | |
| ) as exc: | |
| diagnostic_path = publish_failure_diagnostic( | |
| args.out, | |
| stage="steel-rfq", | |
| input_path=args.input, | |
| error=exc, | |
| tool_version=package_version(__file__), | |
| run_id=args.run_id, | |
| explicit_dates={"issued_date": args.issued_date}, | |
| ) | |
| suffix = ( | |
| f"; diagnostic published: {diagnostic_path}" | |
| if diagnostic_path is not None | |
| else "; diagnostic publication unavailable" | |
| ) | |
| print(f"RFQ generation failed: {exc}{suffix}", file=sys.stderr) | |
| return 1 |
🤖 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 `@skills/steel-rfq/scripts/generate-rfq.py` around lines 1197 - 1215, Extend
the structured exception handling around publish_rfq_run in the main generation
flow to catch RecalculationError alongside the existing OSError, JSON decode,
and input errors. Ensure this exception follows the same
publish_failure_diagnostic, stderr reporting, and exit-1 path, preserving the
documented failure behavior.
|
|
||
| with tarfile.open(archive) as package: | ||
| members = set(package.getnames()) | ||
| package.extractall(tmp_path / "unpacked", filter="data") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for path in pyproject.toml .python-version .tool-versions .github/workflows/ci.yml; do
if [ -f "$path" ]; then
echo "=== $path ==="
rg -n 'requires-python|python-version|PYTHON_VERSION|python:' "$path" || true
fi
doneRepository: StructuPath/pi-steel
Length of output: 387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib, re, sys
files = [
pathlib.Path("tests/test_installed_scripts.py"),
pathlib.Path("pyproject.toml"),
pathlib.Path(".github/workflows/ci.yml"),
]
for path in files:
if path.exists():
print(f"=== {path} ===")
text = path.read_text()
if path.name == "tests/test_installed_scripts.py":
for i, line in enumerate(text.splitlines(), 1):
if "extractall(" in line or "filter=" in line:
print(f"{i}: {line}")
else:
for i, line in enumerate(text.splitlines(), 1):
if re.search(r"requires-python|python-version|python:", line):
print(f"{i}: {line}")
PYRepository: StructuPath/pi-steel
Length of output: 431
Guard this test for Python 3.12+
TarFile.extractall(filter="data") is not available on the repo’s supported Python 3.11 target (requires-python = ">=3.11,<3.14", and CI runs 3.11). Use a 3.11-compatible extraction path or skip this test on older interpreters.
🤖 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 `@tests/test_installed_scripts.py` at line 25, Update the test around
package.extractall to remain compatible with Python 3.11: use the filter
argument only on Python 3.12 and newer, while using the existing compatible
extraction path on older supported interpreters. Preserve the test’s extraction
behavior across the supported Python range.
Summary by CodeRabbit
New Features
Documentation
Tests