Restore corded hangboard assets without silhouette damage - #388
Conversation
This reverts commit 0ae9fc8.
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Reviewer's GuideRestores source-proved hangboard cord assets without silhouette damage, corrects the YY TravelBoard reverse overlay and gravity behavior, removes destructive backdrop-processing infrastructure, and adds tests that prevent image-driven authoring capabilities from returning. Flow diagram for cord asset restoration and policy validationflowchart LR
Evidence[Official product evidence] --> Edit[Built-in asset edit]
Existing[Existing presentation geometry] --> Edit
Edit --> Review[Visual review]
Review -->|accepted| Assets[Restored cord and silhouette assets]
Review -->|rejected| Edit
Assets --> Validate[Package and visual validation]
Policy[Authoring-policy tests] --> Validate
Validate --> Catalog[Validated catalog]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Tools/HangboardPackages/tests/test_authoring_policy.py" line_range="30-35" />
<code_context>
+ return roots
+
+
+def test_production_tooling_has_no_image_driven_authoring_capability() -> None:
+ """Keep raster segmentation, masks, contours, and cropping out of tooling."""
+ violations: list[str] = []
+ for root in PRODUCTION_PYTHON_ROOTS:
+ for path in sorted(root.rglob("*.py")):
+ forbidden = sorted(_imported_roots(path) & FORBIDDEN_IMPORT_ROOTS)
+ if forbidden:
+ violations.append(
</code_context>
<issue_to_address>
**issue (broader_impact):** The authoring-policy guard only rejects four imported module roots and non-dev dependency names; production tooling that performs segmentation, generates masks, or hard-codes pixel cleanup using the standard library or an unlisted/custom image library passes this test. This does not enforce the policy described by the test and allows the deleted destructive authoring path to be reintroduced in a different implementation.
**Triggers:** When a future production script implements image manipulation without importing PIL, cv2, rembg, or skimage.
**Suggested fix:** Add structural checks for the prohibited authoring operations and production-file patterns, or enforce the policy through an explicit allowlist of production modules and capabilities rather than only checking imports and dependency prefixes.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: Tools/HangboardPackages/tests/test_authoring_policy.py:35
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_production_tooling_has_no_image_driven_authoring_capability() -> None: | ||
| """Keep raster segmentation, masks, contours, and cropping out of tooling.""" | ||
| violations: list[str] = [] | ||
| for root in PRODUCTION_PYTHON_ROOTS: | ||
| for path in sorted(root.rglob("*.py")): | ||
| forbidden = sorted(_imported_roots(path) & FORBIDDEN_IMPORT_ROOTS) |
There was a problem hiding this comment.
issue (broader_impact): The authoring-policy guard only rejects four imported module roots and non-dev dependency names; production tooling that performs segmentation, generates masks, or hard-codes pixel cleanup using the standard library or an unlisted/custom image library passes this test. This does not enforce the policy described by the test and allows the deleted destructive authoring path to be reintroduced in a different implementation.
Triggers: When a future production script implements image manipulation without importing PIL, cv2, rembg, or skimage.
Suggested fix: Add structural checks for the prohibited authoring operations and production-file patterns, or enforce the policy through an explicit allowlist of production modules and capabilities rather than only checking imports and dependency prefixes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Tools/HangboardPackages/tests/test_authoring_policy.py">
<violation number="1" location="Tools/HangboardPackages/tests/test_authoring_policy.py:10">
P1: This guard only detects four imported roots, so production tooling can still perform segmentation, mask generation, or pixel cleanup through the standard library or another image library without failing the policy test. Inspect the prohibited operations or enforce an explicit production-capability allowlist instead of relying on this incomplete import list.</violation>
<violation number="2" location="Tools/HangboardPackages/tests/test_authoring_policy.py:34">
P3: The test passes vacuously if `src/` or `scripts/` are renamed or removed: `rglob("*.py")` on a missing directory yields nothing, so no files are scanned and `assert violations == []` still succeeds. A fail-closed policy guard should assert that production files were actually scanned.</violation>
<violation number="3" location="Tools/HangboardPackages/tests/test_authoring_policy.py:44">
P3: The guard only inspects `optional-dependencies` and never checks the top-level `project.dependencies` list. A forbidden package (rembg, Pillow, opencv, scikit-image) added to `dependencies` would bypass the policy even though the README contract says validation is stdlib-only. Check both lists.</violation>
</file>
<file name="docs/source-audits/2026-08-20-complete-hangboard-catalog.md">
<violation number="1" location="docs/source-audits/2026-08-20-complete-hangboard-catalog.md:1113">
P3: The new audit prose contradicts itself about which output is accepted. The first paragraph calls the accepted output "fix-round-2 untouched output" and says it restores cord behind both routing holes, but the fix-round-3 paragraph says the final edit makes the loops "leave their side routing paths and sag downward under gravity." The shipped reverse.png is therefore the fix-round-3 result, not the fix-round-2 output. Reword the first paragraph so the accepted base (fix-round-2) and the final accepted output (fix-round-3) are distinct, or the audit trail misidentifies the shipped asset.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| TOOL_ROOT = Path(__file__).resolve().parents[1] | ||
| PRODUCTION_PYTHON_ROOTS = (TOOL_ROOT / "src", TOOL_ROOT / "scripts") | ||
| FORBIDDEN_IMPORT_ROOTS = frozenset({"PIL", "cv2", "rembg", "skimage"}) |
There was a problem hiding this comment.
P1: This guard only detects four imported roots, so production tooling can still perform segmentation, mask generation, or pixel cleanup through the standard library or another image library without failing the policy test. Inspect the prohibited operations or enforce an explicit production-capability allowlist instead of relying on this incomplete import list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Tools/HangboardPackages/tests/test_authoring_policy.py, line 10:
<comment>This guard only detects four imported roots, so production tooling can still perform segmentation, mask generation, or pixel cleanup through the standard library or another image library without failing the policy test. Inspect the prohibited operations or enforce an explicit production-capability allowlist instead of relying on this incomplete import list.</comment>
<file context>
@@ -0,0 +1,53 @@
+
+TOOL_ROOT = Path(__file__).resolve().parents[1]
+PRODUCTION_PYTHON_ROOTS = (TOOL_ROOT / "src", TOOL_ROOT / "scripts")
+FORBIDDEN_IMPORT_ROOTS = frozenset({"PIL", "cv2", "rembg", "skimage"})
+FORBIDDEN_DEPENDENCY_PREFIXES = (
+ "opencv",
</file context>
| """Keep raster segmentation, masks, contours, and cropping out of tooling.""" | ||
| violations: list[str] = [] | ||
| for root in PRODUCTION_PYTHON_ROOTS: | ||
| for path in sorted(root.rglob("*.py")): |
There was a problem hiding this comment.
P3: The test passes vacuously if src/ or scripts/ are renamed or removed: rglob("*.py") on a missing directory yields nothing, so no files are scanned and assert violations == [] still succeeds. A fail-closed policy guard should assert that production files were actually scanned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Tools/HangboardPackages/tests/test_authoring_policy.py, line 34:
<comment>The test passes vacuously if `src/` or `scripts/` are renamed or removed: `rglob("*.py")` on a missing directory yields nothing, so no files are scanned and `assert violations == []` still succeeds. A fail-closed policy guard should assert that production files were actually scanned.</comment>
<file context>
@@ -0,0 +1,53 @@
+ """Keep raster segmentation, masks, contours, and cropping out of tooling."""
+ violations: list[str] = []
+ for root in PRODUCTION_PYTHON_ROOTS:
+ for path in sorted(root.rglob("*.py")):
+ forbidden = sorted(_imported_roots(path) & FORBIDDEN_IMPORT_ROOTS)
+ if forbidden:
</file context>
| optional_dependencies = pyproject["project"].get("optional-dependencies", {}) | ||
| for extra, dependencies in sorted(optional_dependencies.items()): | ||
| if extra == "dev": | ||
| continue | ||
| for dependency in dependencies: | ||
| normalized = dependency.casefold() | ||
| if normalized.startswith(FORBIDDEN_DEPENDENCY_PREFIXES): | ||
| violations.append(f"optional extra {extra} includes {dependency}") |
There was a problem hiding this comment.
P3: The guard only inspects optional-dependencies and never checks the top-level project.dependencies list. A forbidden package (rembg, Pillow, opencv, scikit-image) added to dependencies would bypass the policy even though the README contract says validation is stdlib-only. Check both lists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Tools/HangboardPackages/tests/test_authoring_policy.py, line 44:
<comment>The guard only inspects `optional-dependencies` and never checks the top-level `project.dependencies` list. A forbidden package (rembg, Pillow, opencv, scikit-image) added to `dependencies` would bypass the policy even though the README contract says validation is stdlib-only. Check both lists.</comment>
<file context>
@@ -0,0 +1,53 @@
+ pyproject = tomllib.loads(
+ (TOOL_ROOT / "pyproject.toml").read_text(encoding="utf-8")
+ )
+ optional_dependencies = pyproject["project"].get("optional-dependencies", {})
+ for extra, dependencies in sorted(optional_dependencies.items()):
+ if extra == "dev":
</file context>
| optional_dependencies = pyproject["project"].get("optional-dependencies", {}) | |
| for extra, dependencies in sorted(optional_dependencies.items()): | |
| if extra == "dev": | |
| continue | |
| for dependency in dependencies: | |
| normalized = dependency.casefold() | |
| if normalized.startswith(FORBIDDEN_DEPENDENCY_PREFIXES): | |
| violations.append(f"optional extra {extra} includes {dependency}") | |
| project = pyproject["project"] | |
| for dependency in project.get("dependencies", []): | |
| normalized = dependency.casefold() | |
| if normalized.startswith(FORBIDDEN_DEPENDENCY_PREFIXES): | |
| violations.append(f"project dependencies include {dependency}") | |
| optional_dependencies = project.get("optional-dependencies", {}) | |
| for extra, dependencies in sorted(optional_dependencies.items()): | |
| if extra == "dev": | |
| continue | |
| for dependency in dependencies: | |
| normalized = dependency.casefold() | |
| if normalized.startswith(FORBIDDEN_DEPENDENCY_PREFIXES): | |
| violations.append(f"optional extra {extra} includes {dependency}") |
| loops and suspension cord behind both routing holes. The built-in image edit | ||
| used the existing reverse presentation as the geometry/composition target and | ||
| YY Vertical's official current-product gallery views 3 and 4 as the physical | ||
| evidence inputs. The accepted fix-round-2 untouched output restores every |
There was a problem hiding this comment.
P3: The new audit prose contradicts itself about which output is accepted. The first paragraph calls the accepted output "fix-round-2 untouched output" and says it restores cord behind both routing holes, but the fix-round-3 paragraph says the final edit makes the loops "leave their side routing paths and sag downward under gravity." The shipped reverse.png is therefore the fix-round-3 result, not the fix-round-2 output. Reword the first paragraph so the accepted base (fix-round-2) and the final accepted output (fix-round-3) are distinct, or the audit trail misidentifies the shipped asset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/source-audits/2026-08-20-complete-hangboard-catalog.md, line 1113:
<comment>The new audit prose contradicts itself about which output is accepted. The first paragraph calls the accepted output "fix-round-2 untouched output" and says it restores cord behind both routing holes, but the fix-round-3 paragraph says the final edit makes the loops "leave their side routing paths and sag downward under gravity." The shipped reverse.png is therefore the fix-round-3 result, not the fix-round-2 output. Reword the first paragraph so the accepted base (fix-round-2) and the final accepted output (fix-round-3) are distinct, or the audit trail misidentifies the shipped asset.</comment>
<file context>
@@ -1103,7 +1103,34 @@ Reverse prompt (verbatim):
+loops and suspension cord behind both routing holes. The built-in image edit
+used the existing reverse presentation as the geometry/composition target and
+YY Vertical's official current-product gallery views 3 and 4 as the physical
+evidence inputs. The accepted fix-round-2 untouched output restores every
+source-proved cord segment—both short side loops and cord behind both routing
+holes—on the original 1774 × 887 opaque canvas.
</file context>
|
The Hangboard Workbench build for |
1 participant
Summary
Catalog audit
The cord/gravity audit covers 47 relevant presentations. Six are currently correct; 41 remain explicitly blocked because the available editor could not preserve exact product pixels, alpha, canvas, framing, and source topology. No rejected bulk-generated assets remain in the final tree.
Validation
Summary by Sourcery
Restore source-correct hangboard cord and silhouette assets while removing destructive image-processing authoring paths.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Summary by cubic
Restores five hangboard presentations that lost their source-proved cords and full silhouettes to a destructive backdrop-removal step, and removes that pipeline so it can't damage future assets. The catalog audit covers 47 presentations (6 correct, 41 still blocked by editor limits, zero rejected bulk-generated assets), and all 394 HangboardPackages tests plus the iOS visual harness pass.
Bug Fixes
reverse-10edge frame inboard.jsonto stay inside the recess.Refactors
remove_primary_backdrops.py, therembgdependency, its fixture, and its tests; no remaining code imports them.test_authoring_policy.pythat fails if production tooling imports image-processing packages.Written for commit 90b85ce. Summary will update on new commits.