integrate paco-classifier staffline detection; fix neume/syllable positioning in neon - #173
Open
giannatan wants to merge 14 commits into
Open
integrate paco-classifier staffline detection; fix neume/syllable positioning in neon#173giannatan wants to merge 14 commits into
giannatan wants to merge 14 commits into
Conversation
…ction paco's is run on the original/input images -> staffline png is fed to .pt model, isolating stave boxes -> passed to staffline detection model (previously wired in)
…d of hardcoded dict
… processing page)
…lting in early false completion of processing for larger images
…oding step known bugs, to be fixed: - syllables incorrectly being linked to neumes on the stave below the syllable - incorrectly blank syllables and unpaired solo neumes - order of syllables in neon goes back and forth between adjacent lines, rather than line by line
…d to the neumes in the staff above it
…curacy and placement
Notes on some staves were visibly floating above or below their real ink, worst on staves whose detected line count didn't match the nominal 4. Three compounding causes, all in the stave/pitch pipeline between staffline detection and MEI output: - staves_from_jsomr() sampled left/right fragments of one real ruled line as if they were separate lines (the detector splits a line into two records when something interrupts it mid-page), inflating line_ys on affected staves from 4 to as many as 7 entries. Add _dedupe_line_ys() to collapse near-duplicate samples via a bimodal split in the sorted gap distribution, guarded so it never merges genuinely uniform real lines. - _step_from_y() indexed line_ys[len(line_ys) - clef_line] to find the clef line, which silently assumed len(line_ys) always equals the real line count. On real pages that count ranged from 2 to 7 for a nominal 4-line stave, so the index could land on the wrong line entirely, or even wrap negative. Anchor clef_y from the bottom detected line and extrapolate by the median gap instead, matching MEI's own @line-from-bottom convention. Also switch line_spacing from mean to median so remaining outlier gaps can't drag it off. - assign_glyphs_to_staves's missed-stave recovery synthesized line_ys from the missed row's own glyph bounding box, which a single unusually high/low note can badly skew. _typical_line_spacing() borrows the page's own already-detected spacing instead. - Verovio's own facsimile rendering derives each staff's per-note pixel spacing purely from the stave zone's height (zone_height / (lines - 1)), independently of line_ys — a separate computation from _step_from_y's pitch assignment. The zone heights this file wrote (padded, or a union of detected lines' own bounding boxes) implied a spacing up to 2x off the real one, so a note's correctly-assigned diatonic step still rendered at the wrong pixel distance from the clef. _stave_zone_bounds() now sizes the zone to exactly (STAFF_LINES - 1) * real spacing, anchored the same way _step_from_y is, so the two always agree.
📝 WalkthroughWalkthroughThis PR adds the Paco classifier service, integrates staffline separation with medieval inference, updates mapping-based MEI encoding, adds project job coordination, and reformats frontend code. ChangesPaco classification and medieval processing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessingTask
participant YoloModelSet
participant PacoAPI
participant PacoService
participant MEIEncoder
ProcessingTask->>YoloModelSet: run text and music inference
ProcessingTask->>PacoAPI: submit page image
PacoAPI->>PacoService: POST /classify
PacoService-->>PacoAPI: return staffline and background PNGs
PacoAPI-->>ProcessingTask: provide decoded layers
ProcessingTask->>YoloModelSet: detect staves from staffline layer
ProcessingTask->>MEIEncoder: pass detected staves and alignment data
MEIEncoder-->>ProcessingTask: return verified MEI
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-07T22:04:44Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: cloudformation scan error: fs filter error: fs filter error: walk error range error: stat .coderabbit-opengrep-fallback.897007a8-9316-4e44-a07c-1fdd5f1137f0.yml: no such file or directory: range error: stat .coderabbit-opengrep-fallback.897007a8-9316-4e44-a07c-1fdd5f1137f0.yml: no such file or directory 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
landing-page/src/hooks/useActiveJobWatcher.ts-16-24 (1)
16-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent duplicate terminal callbacks.
If a poll takes longer than five seconds, another interval can poll the same job before the first poll completes. Both responses can report a terminal status.
markJobSettled()removes the job, but the second loop still callsonJobDone(). This can show duplicate completion notifications.Make settlement report whether it removed the job, or track jobs currently being settled. Call
onJobDone()only for the loop that settled the job.🤖 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 `@landing-page/src/hooks/useActiveJobWatcher.ts` around lines 16 - 24, Prevent duplicate terminal callbacks in the interval watcher by making markJobSettled report whether it actually removed the job, or by tracking jobs currently being settled; invoke onJobDone only when the current polling loop successfully settles that job, while preserving the existing terminal-status handling.dev.sh-41-41 (1)
41-41: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the backend URL synchronized with custom
PACO_PORTvalues.
PACO_API_URLis still based on the configured defaulthttp://localhost:8003. If a developer changesPACO_PORT,dev.shstarts Paco on the new port while the backend may continue using the configured default URL. Set the default fromPACO_PORT, or passPACO_API_URLwith the same port when starting the worker. Preserve any explicitPACO_API_URLenvironment override.🤖 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 `@dev.sh` at line 41, Update the dev.sh configuration around PACO_PORT so the default PACO_API_URL uses the effective PACO_PORT value instead of hardcoding 8003. Preserve any explicitly provided PACO_API_URL override while keeping the worker’s backend URL synchronized with custom ports.Source: Coding guidelines
landing-page/scripts/auth_api.py-775-777 (1)
775-777: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the
dictreturn contract forget_latest_text_alignment().
json.loads()accepts arrays, strings, numbers, and objects. A non-dictionary value is truthy and returned from this function, while callers use.get(), so a misshapedtext_alignmentsrow can fail encoding instead of using the fallback. Validate that parsedalignment_jsonis adict, or reject non-dictionary values there.🤖 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 `@landing-page/scripts/auth_api.py` around lines 775 - 777, Update get_latest_text_alignment() so the value parsed from row[0] is returned only when it is a dict; reject non-dictionary JSON values and allow the existing fallback path to handle them. Preserve the current behavior for valid dictionary payloads.paco-classifier-service/main.py-33-35 (1)
33-35: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRestrict the CORS origins for this service.
The service has no authentication and accepts any origin, method, and header. Any web page can post images to it, if the port is reachable from a browser. The Mothra worker calls this service server-to-server, so CORS is not required for that path.
Remove the middleware, or set
allow_originsto the known frontend origin.🤖 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 `@paco-classifier-service/main.py` around lines 33 - 35, Restrict or remove the CORSMiddleware configuration in the application setup; do not permit wildcard origins, methods, or headers. Since the Mothra worker uses server-to-server calls, remove the middleware unless browser access is required; otherwise configure allow_origins with only the known frontend origin.landing-page/scripts/tests/test_encode_to_mei.py-279-279 (1)
279-279: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the three lint errors so a Ruff-gated CI stays green.
Ruff reports
F541at lines 279 and 348. The stringf"{{http://www.w3.org/XML/1998/namespace}}id"contains only escaped braces, so thefprefix is unused. Ruff reportsRUF059at line 403, becauseglyphs_by_staveis unpacked and never read.♻️ Proposed changes
- gid = neume.get(f"{{http://www.w3.org/XML/1998/namespace}}id").removeprefix("neume-") + gid = neume.get("{http://www.w3.org/XML/1998/namespace}id").removeprefix("neume-")- glyphs_by_stave, staves = mei.assign_glyphs_to_staves( + _glyphs_by_stave, staves = mei.assign_glyphs_to_staves( missed_row_glyphs, [detected], page_w=1000, page_h=1000, )Define the XML id attribute name once as a module constant, next to
MEI_NS, and reuse it at both sites.Also applies to: 348-348, 403-403
🤖 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 `@landing-page/scripts/tests/test_encode_to_mei.py` at line 279, Fix the Ruff violations in the XML ID and glyph grouping logic: define a module-level XML ID attribute constant beside MEI_NS, use it at both neume.get sites instead of the unused f-string prefix, and remove or otherwise avoid unpacking the unused glyphs_by_stave value in the affected loop.Source: Linters/SAST tools
landing-page/scripts/staffline_adapter.py-86-91 (1)
86-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExactly coincident samples defeat the split test.
small_side <= 0returns the input unchanged. Two fragments of one ruled line can sample the identical y, for example when both fragments clamp to the same fitted value. That produces a gap of exactly0.0. If that zero gap is also the smaller side at the biggest jump, for example gaps[0.0, 30.0, 30.0], the function returns early and the duplicate line stays inline_ys. That is the same inflated line count the function exists to remove, and it feeds_step_from_y's clef lookup inencode_to_mei.py.Treat a zero smaller side as a valid duplicate cluster instead of a bail-out. Keep the bail-out only for a negative value, which cannot occur on a sorted list.
🐛 Proposed fix
- if biggest_jump <= 0 or small_side <= 0 or large_side < small_side * 3: + if biggest_jump <= 0 or small_side < 0 or (small_side > 0 and large_side < small_side * 3): return line_ys # no clear bimodal split -- treat every gap as realAdd a test with a coincident pair, for example
[100.0, 100.0, 130.0, 160.0].🤖 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 `@landing-page/scripts/staffline_adapter.py` around lines 86 - 91, Update the split guard in the function containing the sorted_gaps and biggest_jump logic to reject only negative small_side values, allowing zero to proceed as a valid duplicate cluster. Preserve the existing threshold calculation and other bimodal checks, and add coverage for coincident samples such as [100.0, 100.0, 130.0, 160.0] to verify the duplicate line is removed.
🧹 Nitpick comments (4)
paco-classifier-service/Dockerfile (1)
15-18: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider running the service as a non-root user.
The image runs
uvicornas root. Add a dedicated user to reduce container privilege. This is an internal service, so the risk is limited.♻️ Proposed change
COPY paco-classifier/ ./paco-classifier/ WORKDIR /app/paco-classifier-service +RUN useradd --create-home --uid 10001 appuser && chown -R appuser /app +USER appuser EXPOSE 8003 CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "8003"]🤖 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 `@paco-classifier-service/Dockerfile` around lines 15 - 18, Add a dedicated non-root user in the Dockerfile, ensure the application files are accessible to it, and switch to that user before the existing uvicorn CMD. Preserve the current WORKDIR, exposed port, and service startup command.landing-page/scripts/encode_to_mei.py (2)
603-613: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
STAFF_LINESin the synthesized line positions.This branch hardcodes
range(4)and the1.5centre offset. Line 47 addsSTAFF_LINESfor exactly this purpose, and_stave_zone_boundsalready uses it. IfSTAFF_LINESchanges, this branch silently disagrees with the zone bounds and with<staffDef lines>.♻️ Proposed change
center_y = median(g.cy for g in row) - line_ys = [center_y + typical_line_spacing * (j - 1.5) for j in range(4)] + offset = (STAFF_LINES - 1) / 2 + line_ys = [ + center_y + typical_line_spacing * (j - offset) + for j in range(STAFF_LINES) + ] else: - line_ys = [est_uly + h * j / 3 for j in range(4)] + line_ys = [ + est_uly + h * j / (STAFF_LINES - 1) for j in range(STAFF_LINES) + ]🤖 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 `@landing-page/scripts/encode_to_mei.py` around lines 603 - 613, Update the typical_line_spacing branch in the row line-position synthesis to use the STAFF_LINES constant for both the number of generated lines and the center offset, matching _stave_zone_bounds and <staffDef lines> behavior when STAFF_LINES changes.
362-362: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
strict=Trueor validate the paired inputs in thesezip()loops.
zip()silently drops extra syllables or note components ifstave_boxes/glyph_groups,pooled_boxes/pooled_glyph_groups, orcomponents/pitchesare not the same length. A short invariant check orzip(..., strict=True)makes that misalignment fail instead of changing the generated input.🤖 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 `@landing-page/scripts/encode_to_mei.py` at line 362, Update the zip loops producing paired syllable, box, glyph, component, and pitch data—including the flow returning from stave_boxes and glyph_groups—to use strict length validation, preferably zip(..., strict=True), or equivalent explicit checks before zipping. Ensure mismatched stave_boxes/glyph_groups, pooled_boxes/pooled_glyph_groups, or components/pitches fail rather than silently dropping entries.Source: Linters/SAST tools
landing-page/scripts/tests/test_encode_to_mei.py (1)
374-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
verify_and_correct_syllablesand_stave_zone_bounds.This file covers the new spacing, clef, and row-grouping helpers well. Two new, high-risk functions have no test.
verify_and_correct_syllablesrewrites persisted MEI in place and removes existing<syllable>and<zone>elements.mei_api.pycalls it on every edit-session creation for an uncorrected file. A round-trip test is valuable: build an MEI withbuild_mei, feed it back with the sametext_alignment, and assert that nothing changes. A second test with fragmented staves would show the divergence raised separately onencode_to_mei.py._stave_zone_boundsnow controls Verovio's rendered pixel spacing. Assert that the returned height equals(STAFF_LINES - 1) * spacing, and that a stave with fewer than twoline_ysfalls back to the raw bbox.Do you want me to draft these tests?
As per coding guidelines: "Maintain and run both the
staff-findingalgorithmic test suite andlanding-page/scripts/tests/in CI."🤖 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 `@landing-page/scripts/tests/test_encode_to_mei.py` around lines 374 - 411, Add tests in the existing test module for verify_and_correct_syllables: build MEI with build_mei, run correction with the same text_alignment, and assert the round trip is unchanged; also cover fragmented staves as appropriate. Add _stave_zone_bounds tests asserting height equals (STAFF_LINES - 1) * spacing for valid line_ys and that fewer than two line_ys uses the raw bounding-box fallback. Ensure both the staff-finding algorithmic suite and landing-page/scripts/tests remain covered in CI.Source: Coding guidelines
🤖 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 @.github/workflows/tests.yml:
- Around line 66-70: Update the workflow steps around the existing landing-page
pytest commands to add a pytest invocation for the complete
landing-page/scripts/tests/ directory, ensuring files such as
test_encode_to_mei.py run in CI. Preserve the targeted staffline_adapter and
paco_api steps only if they provide distinct isolation, and retain the existing
staff-finding algorithmic test suite.
In `@docker-compose.yml`:
- Around line 41-42: Replace the classifier service’s host-published ports entry
with expose for internal Compose-only access, preserving port 8003 for backend
and worker through the paco-classifier-service hostname; only bind to 127.0.0.1
if local host access is explicitly required.
- Around line 68-69: Make paco-classifier-service expose a healthcheck endpoint
that verifies classifier initialization is complete, configure Docker Compose to
use that healthcheck, and change both backend and worker dependency conditions
from service_started to service_healthy. Preserve raw-page detection only as the
runtime fallback after the classifier readiness check is unavailable or fails.
In `@k8s/paco-classifier-service.yaml`:
- Around line 17-23: Add explicit pod and container security contexts to the
Deployment for paco-classifier-service: set runAsNonRoot: true at the pod level,
and set allowPrivilegeEscalation: false plus capabilities.drop: ["ALL"] on the
paco-classifier-service container. Ensure the configuration enforces these
controls independently of namespace defaults.
- Around line 32-41: Update the readinessProbe for the classifier service to use
an HTTP endpoint that verifies recognition_engine.process_image_msae()
dependencies and model availability, rather than only checking whether port 8003
is open. Keep the livenessProbe behavior unchanged, and ensure the readiness
endpoint reports failure until startup checks and classifier dependencies are
usable.
In `@landing-page/scripts/auth_api.py`:
- Around line 778-779: Update get_latest_text_alignment to distinguish an absent
alignment row from malformed JSON or invalid data shape: handle the missing-row
case explicitly, catch only the expected JSON/data-shape exceptions, and let
database exceptions propagate. Before releasing the connection on a database
failure, roll back the owning database context so callers such as
tasks_encode.py and mei_api.py receive the operational error.
- Around line 770-774: Update get_latest_text_alignment to resolve alignments by
the image_id stored in text_alignments rather than the non-unique
image_name/project_id pair; propagate the image identifier from its callers,
including the /text/run and /text-batch/run flows, and keep the
latest-created-row behavior for that image.
In `@landing-page/scripts/encode_to_mei.py`:
- Around line 1332-1337: Update verify_and_correct_syllables to preserve the
existing syllable text when no glyph signature changed, rather than replacing it
with the "-" fallback. Account for reconstructed staves from
_reconstruct_mei_state having tighter staff-zone bounds that may place assigned
syllable boxes outside the encoded band, while retaining the original row-mate
fragment stave text.
In `@landing-page/scripts/job_store.py`:
- Around line 71-82: In landing-page/scripts/job_store.py lines 71-82, add an
atomic project-job claim operation that uses a per-project transaction lock,
checks all active jobs regardless of kind, and rejects or explicitly reuses an
existing active job before inserting; commit the lock, check, and insert in one
transaction. In landing-page/scripts/batch_api.py lines 92-103 and
landing-page/scripts/inference_api.py lines 46-57, replace the separate
active-job lookup and create_job calls with this claim operation, preserving the
existing response behavior for rejected or reused jobs.
In `@landing-page/scripts/mei_api.py`:
- Around line 143-158: Wrap the silent re-sync logic in create_edit_session
around encode_to_mei.image_dimensions and
encode_to_mei.verify_and_correct_syllables with exception handling so malformed
images or XML do not propagate and block the edit session. On failure, preserve
the prior behavior by skipping correction and continuing without updating the
database; keep successful correction persistence and logging unchanged.
In `@landing-page/scripts/tasks_predict.py`:
- Around line 94-118: Call check_cancelled(job_id) immediately after the
classifier thread wait loop exits and before the if "error" in stave_result
block, so cancellation is revalidated before processing results or returning
annotation data.
In `@landing-page/src/components/workflow/NeonBatchEditor.tsx`:
- Around line 170-173: Replace the inline style object on the button in
NeonBatchEditor with Tailwind CSS v4 utility classes, including the conditional
opacity based on nearestUncorrected(currentIndex, -1). Remove the btn style
spread and preserve the existing visual styling and disabled-state behavior
through className utilities.
In `@landing-page/src/components/workflow/ProcessingPage.tsx`:
- Around line 349-363: Update handleRetryJob after assigning jobIdRef.current to
newId so it registers the retried job by calling registerActiveJobs with newId
before starting stream consumption. Preserve the existing retry initialization
and ensure the watcher continues tracking the new job until completion.
In `@landing-page/src/lib/apiFetch.ts`:
- Around line 27-58: Update the 401 handling in apiFetch to serialize
refresh-token rotation with one module-level in-flight refresh promise. Make
concurrent handlers await the shared refresh operation and reuse its resulting
access token when retrying, clearing credentials and notifying unauthenticated
only when that shared refresh fails; preserve the existing no-refresh-token
behavior.
In `@paco-classifier-service/main.py`:
- Around line 95-119: Make classify a synchronous def endpoint, or move the
blocking process_image_msae and _layer_to_rgba_png calls to run_in_threadpool so
TensorFlow, NumPy, OpenCV, and PNG work do not block the event loop. If
recognition_engine is not thread-safe, protect concurrent process_image_msae
calls with the appropriate lock.
In `@paco-classifier-service/requirements.txt`:
- Around line 1-19: Update the dependency declarations in requirements.txt:
raise the python-multipart minimum from 0.0.9 to 0.0.30 or newer, and add h11
with a minimum version of 0.16.0 to cover the multipart upload path used by
/classify.
---
Minor comments:
In `@dev.sh`:
- Line 41: Update the dev.sh configuration around PACO_PORT so the default
PACO_API_URL uses the effective PACO_PORT value instead of hardcoding 8003.
Preserve any explicitly provided PACO_API_URL override while keeping the
worker’s backend URL synchronized with custom ports.
In `@landing-page/scripts/auth_api.py`:
- Around line 775-777: Update get_latest_text_alignment() so the value parsed
from row[0] is returned only when it is a dict; reject non-dictionary JSON
values and allow the existing fallback path to handle them. Preserve the current
behavior for valid dictionary payloads.
In `@landing-page/scripts/staffline_adapter.py`:
- Around line 86-91: Update the split guard in the function containing the
sorted_gaps and biggest_jump logic to reject only negative small_side values,
allowing zero to proceed as a valid duplicate cluster. Preserve the existing
threshold calculation and other bimodal checks, and add coverage for coincident
samples such as [100.0, 100.0, 130.0, 160.0] to verify the duplicate line is
removed.
In `@landing-page/scripts/tests/test_encode_to_mei.py`:
- Line 279: Fix the Ruff violations in the XML ID and glyph grouping logic:
define a module-level XML ID attribute constant beside MEI_NS, use it at both
neume.get sites instead of the unused f-string prefix, and remove or otherwise
avoid unpacking the unused glyphs_by_stave value in the affected loop.
In `@landing-page/src/hooks/useActiveJobWatcher.ts`:
- Around line 16-24: Prevent duplicate terminal callbacks in the interval
watcher by making markJobSettled report whether it actually removed the job, or
by tracking jobs currently being settled; invoke onJobDone only when the current
polling loop successfully settles that job, while preserving the existing
terminal-status handling.
In `@paco-classifier-service/main.py`:
- Around line 33-35: Restrict or remove the CORSMiddleware configuration in the
application setup; do not permit wildcard origins, methods, or headers. Since
the Mothra worker uses server-to-server calls, remove the middleware unless
browser access is required; otherwise configure allow_origins with only the
known frontend origin.
---
Nitpick comments:
In `@landing-page/scripts/encode_to_mei.py`:
- Around line 603-613: Update the typical_line_spacing branch in the row
line-position synthesis to use the STAFF_LINES constant for both the number of
generated lines and the center offset, matching _stave_zone_bounds and <staffDef
lines> behavior when STAFF_LINES changes.
- Line 362: Update the zip loops producing paired syllable, box, glyph,
component, and pitch data—including the flow returning from stave_boxes and
glyph_groups—to use strict length validation, preferably zip(..., strict=True),
or equivalent explicit checks before zipping. Ensure mismatched
stave_boxes/glyph_groups, pooled_boxes/pooled_glyph_groups, or
components/pitches fail rather than silently dropping entries.
In `@landing-page/scripts/tests/test_encode_to_mei.py`:
- Around line 374-411: Add tests in the existing test module for
verify_and_correct_syllables: build MEI with build_mei, run correction with the
same text_alignment, and assert the round trip is unchanged; also cover
fragmented staves as appropriate. Add _stave_zone_bounds tests asserting height
equals (STAFF_LINES - 1) * spacing for valid line_ys and that fewer than two
line_ys uses the raw bounding-box fallback. Ensure both the staff-finding
algorithmic suite and landing-page/scripts/tests remain covered in CI.
In `@paco-classifier-service/Dockerfile`:
- Around line 15-18: Add a dedicated non-root user in the Dockerfile, ensure the
application files are accessible to it, and switch to that user before the
existing uvicorn CMD. Preserve the current WORKDIR, exposed port, and service
startup command.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e7719222-96c3-4503-b900-9a18e43e575e
⛔ Files ignored due to path filters (2)
landing-page/scripts/assets/mei_encoding/hufnagel.csvis excluded by!**/*.csvlanding-page/scripts/assets/mei_encoding/square.csvis excluded by!**/*.csv
📒 Files selected for processing (78)
.github/workflows/build-images.yml.github/workflows/tests.yml.gitignore.gitmodulesCLAUDE.mddev.shdocker-compose.ymlick8s/configmap.yamlk8s/paco-classifier-service.yamllanding-page/eslint.config.tslanding-page/scripts/auth_api.pylanding-page/scripts/batch_api.pylanding-page/scripts/config.pylanding-page/scripts/config.yamllanding-page/scripts/encode_to_mei.pylanding-page/scripts/inference_api.pylanding-page/scripts/job_store.pylanding-page/scripts/mei_api.pylanding-page/scripts/neume_mapping.pylanding-page/scripts/paco_api.pylanding-page/scripts/staffline_adapter.pylanding-page/scripts/tasks_encode.pylanding-page/scripts/tasks_predict.pylanding-page/scripts/tests/test_encode_to_mei.pylanding-page/scripts/tests/test_paco_api.pylanding-page/scripts/tests/test_staffline_adapter.pylanding-page/scripts/yolo_inference.pylanding-page/src/App.tsxlanding-page/src/components/AppRouter.tsxlanding-page/src/components/documentation/DocsQuickStart.tsxlanding-page/src/components/documentation/DocsWalkthrough.tsxlanding-page/src/components/project/AnnotationViewerModal.tsxlanding-page/src/components/project/BatchFolioReviewModal.tsxlanding-page/src/components/project/BatchTab.tsxlanding-page/src/components/project/CantusSourcePanel.tsxlanding-page/src/components/project/EditFolioModel.tsxlanding-page/src/components/project/ImageTab.tsxlanding-page/src/components/project/LargeImageWarningModal.tsxlanding-page/src/components/project/MeiTab.tsxlanding-page/src/components/project/MeiViewerModal.tsxlanding-page/src/components/project/ModelTab.tsxlanding-page/src/components/project/MyProjects.tsxlanding-page/src/components/project/ProjectDetail.tsxlanding-page/src/components/project/RhythmChart.tsxlanding-page/src/components/project/StafflineViewerModal.tsxlanding-page/src/components/project/TextAlignmentViewerModal.tsxlanding-page/src/components/project/TextAlignmentsTab.tsxlanding-page/src/components/shared/AssetGrid.tsxlanding-page/src/components/shared/ContextMenu.tsxlanding-page/src/components/shared/Modal.tsxlanding-page/src/components/shared/ToastContainer.tsxlanding-page/src/components/workflow/CompletionPage.tsxlanding-page/src/components/workflow/InteractiveClassifier.tsxlanding-page/src/components/workflow/MeiCompareModal.tsxlanding-page/src/components/workflow/MeiImageDiffView.tsxlanding-page/src/components/workflow/NeonBatchEditor.tsxlanding-page/src/components/workflow/NeonCompletionPage.tsxlanding-page/src/components/workflow/ProcessingPage.tsxlanding-page/src/hooks/useActiveJobWatcher.tslanding-page/src/hooks/useAuth.tslanding-page/src/hooks/useEncodingFlow.tslanding-page/src/hooks/useInferenceSettings.tslanding-page/src/hooks/useProjectMutations.tslanding-page/src/hooks/useTextFindingSettings.tslanding-page/src/hooks/useZoomPan.tslanding-page/src/lib/activeJobs.tslanding-page/src/lib/apiFetch.tslanding-page/src/lib/rhythmGaps.tslanding-page/src/lib/toast.tslanding-page/src/utils/folio.tslanding-page/src/utils/imageResize.tslanding-page/src/utils/imageStep.tslanding-page/src/utils/meiZoneDiff.tspaco-classifierpaco-classifier-service/Dockerfilepaco-classifier-service/main.pypaco-classifier-service/requirements.txt
Comment on lines
+170
to
+173
| style={{ | ||
| ...btn, | ||
| opacity: nearestUncorrected(currentIndex, -1) === -1 ? 0.4 : 1, | ||
| }} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace inline styles with Tailwind CSS v4 classes.
Line 170 continues to use inline styles. Migrate this component’s style objects to Tailwind CSS v4 utility classes before merge. As per coding guidelines, “Use Tailwind CSS v4 classes for styling; do not use CSS modules or inline styles.”
🤖 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 `@landing-page/src/components/workflow/NeonBatchEditor.tsx` around lines 170 -
173, Replace the inline style object on the button in NeonBatchEditor with
Tailwind CSS v4 utility classes, including the conditional opacity based on
nearestUncorrected(currentIndex, -1). Remove the btn style spread and preserve
the existing visual styling and disabled-state behavior through className
utilities.
Source: Coding guidelines
# Conflicts: # .github/workflows/tests.yml # ic # landing-page/scripts/encode_to_mei.py # landing-page/scripts/inference_api.py # landing-page/scripts/tasks_encode.py # landing-page/src/components/AppRouter.tsx # landing-page/src/components/project/BatchFolioReviewModal.tsx # landing-page/src/components/project/BatchTab.tsx # landing-page/src/components/project/CantusSourcePanel.tsx # landing-page/src/components/project/EditFolioModel.tsx # landing-page/src/components/project/ImageTab.tsx # landing-page/src/components/project/MeiViewerModal.tsx # landing-page/src/components/project/RhythmChart.tsx # landing-page/src/components/project/StafflineViewerModal.tsx # landing-page/src/components/project/TextAlignmentViewerModal.tsx # landing-page/src/components/workflow/ProcessingPage.tsx # landing-page/src/hooks/useEncodingFlow.ts # landing-page/src/hooks/useTextFindingSettings.ts # landing-page/src/utils/folio.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
landing-page/scripts/tests/test_encode_to_mei.py (1)
403-411: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the recovered-spacing assertion non-vacuous.
The call uses
allow_synthetic_lines=Falseby default. The recovered stave then has an emptyline_yslist.all(...)on the resulting empty spacing list returnsTrue.Pass
allow_synthetic_lines=Trueand assert the expected line count before checking spacing.Proposed fix
_glyphs_by_stave, staves = mei.assign_glyphs_to_staves( - missed_row_glyphs, [detected], page_w=1000, page_h=1000, + missed_row_glyphs, [detected], page_w=1000, page_h=1000, + allow_synthetic_lines=True, ) recovered = staves[1] +assert len(recovered.line_ys) == mei.STAFF_LINES spacings = [recovered.line_ys[i + 1] - recovered.line_ys[i] for i in range(len(recovered.line_ys) - 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 `@landing-page/scripts/tests/test_encode_to_mei.py` around lines 403 - 411, Update the assign_glyphs_to_staves call in the test to pass allow_synthetic_lines=True, then assert the recovered stave has the expected number of lines before computing spacings; retain the existing 20px spacing assertion so it cannot pass on an empty line_ys list.landing-page/scripts/tasks_encode.py (1)
99-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoll back after a suppressed database error.
Lines 99 and 118 suppress query errors but leave the PostgreSQL transaction aborted. The next query can fail, and
release_db_conn(con)can return an unusable connection to the pool.Call
con.rollback()in both handlers before continuing with the fallback. Keep theget_latest_text_alignment()handler separate because that helper already rolls back.Proposed fix
- except Exception: - pass + except Exception: + con.rollback() ... - except Exception: - pass + except Exception: + con.rollback()Also applies to: 118-119
🤖 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 `@landing-page/scripts/tasks_encode.py` around lines 99 - 100, Add con.rollback() to both exception handlers that suppress database query errors in the affected task flow, before continuing with fallback processing or releasing the connection. Keep the get_latest_text_alignment() handler separate and do not add a duplicate rollback there because that helper already performs it.landing-page/src/components/shared/FolioSelect.tsx (1)
15-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDecouple the displayed custom input from the parent
value.
FolioSelectaccepts an invalid custom entry but only updates localcustomInput; the caller still shows the previous committedvalue. That lets the picker display a bad folio while callers use the old value. Emit""or an explicit invalid/falsy token for invalid custom input, or validate at submission.🤖 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 `@landing-page/src/components/shared/FolioSelect.tsx` around lines 15 - 21, Update FolioSelect’s custom-input change handling so invalid or unrecognized folio text cannot remain displayed while the parent value retains the previous valid selection. When customInput is invalid, propagate an empty or other explicit falsy invalid value through onChange (or validate before submission), while preserving normal propagation for valid custom and canonical folio selections.
♻️ Duplicate comments (1)
paco-classifier-service/main.py (1)
114-131:⚠️ Potential issue | 🟠 MajorBound concurrent model loads.
def classify()correctly runs outside the event loop, but FastAPI can dispatch several calls to its thread pool. Each call invokesprocess_image_msae()with both model paths, and the service reloads those models for each request. The Kubernetes limit is 4 GiB; the Compose worker can submit two calls concurrently, and backend calls can add more. Concurrent full-page requests can cause OOM kills or severe latency.Add a process-wide bounded semaphore or queue around classification. Return a controlled
503or429when capacity is full, and set capacity from measured peak memory and verified engine thread safety. This is the concurrent-call gap noted in the previous review, and it remains present.This follows the supplied FastAPI endpoint, worker concurrency, and Kubernetes resource context.
Verification command
#!/bin/bash set -euo pipefail rg -n -C 5 \ 'def classify|process_image_msae|pool=threads|concurrency|memory: 4Gi' \ paco-classifier-service/main.py docker-compose.yml k8s/paco-classifier-service.yaml🤖 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 `@paco-classifier-service/main.py` around lines 114 - 131, Add a process-wide bounded semaphore around the model invocation in classify, limiting concurrent process_image_msae calls to the capacity validated against memory usage and engine thread safety. Acquire it without blocking indefinitely; when unavailable, return an HTTP 503 or 429 response, while preserving the existing image decoding and successful classification flow.
🧹 Nitpick comments (2)
landing-page/scripts/inference_api.py (1)
198-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix the unused
conbinding with an underscore.Ruff reports RUF059 for the unused
conunpacked at Line 198. The adjacent unpacked names already use the underscore convention. Apply the same convention here to keep the lint clean.♻️ Proposed change
- with db_cursor() as (con, cur): + with db_cursor() as (_con, cur): _image_id, image_name, _ann_id, image_arr, yolo_txt = _load_image_and_yolo_for_detection( cur, project_id, detection_id, user["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 `@landing-page/scripts/inference_api.py` around lines 198 - 201, Rename the unused con binding in the db_cursor() context manager unpacking to an underscore, matching the existing convention for intentionally unused values and resolving Ruff RUF059.Source: Linters/SAST tools
landing-page/scripts/job_store.py (1)
83-88: 🚀 Performance & Scalability | 🔵 TrivialConsider an index for the active-job lookup.
Both queries in this function filter
jobsbyproject_idandstatus, and they now run on every kickoff request. A partial index on(project_id, created_at DESC)restricted tostatus IN ('pending','running')keeps the lookup cheap as thejobstable grows.[operational]
🤖 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 `@landing-page/scripts/job_store.py` around lines 83 - 88, Update the database schema or migration used by the job store to add a partial index on jobs covering project_id and created_at DESC, restricted to rows whose status is pending or running. Ensure it supports both active-job queries in the function, including the ORDER BY created_at DESC lookup.
🤖 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 `@landing-page/scripts/job_store.py`:
- Around line 83-93: Update the active-job query in the surrounding job-store
function to filter out allowed kinds in SQL and select the newest remaining
conflicting active job, rather than applying ORDER BY/LIMIT before checking the
fetched row. Preserve the existing None result when no conflicting job exists
and return the conflicting job metadata unchanged.
In `@landing-page/src/components/shared/FolioSelect.tsx`:
- Around line 31-39: Update the local state synchronization in FolioSelect so
customInput tracks subsequent value changes whenever customMode is active,
rather than only deriving its initial value at mount. Add the necessary
value-dependent effect or equivalent update, and reset customInput when the
value is cleared while preserving existing behavior for canonical values and
mode transitions.
In `@landing-page/src/utils/folio.ts`:
- Around line 274-278: Update the fallback branch that pushes nextCanonical so c
(canonicalConsumed) increments only when nextCanonical is defined; preserve
sequence and isPhantom updates while preventing surplus uploads from advancing
beyond canonical.length.
In `@paco-classifier-service/main.py`:
- Around line 97-110: Separate the current health check into liveness and
readiness endpoints, keeping liveness lightweight while making readiness
validate that both classifier models can load successfully or use an explicitly
initialized engine state. Update the Compose and Kubernetes probe targets to use
the readiness endpoint, and ensure classify() relies on the same validated
readiness state before calling recognition_engine.process_image_msae().
- Around line 114-121: Update the classify handler to enforce a maximum upload
size while reading image.file, returning HTTP 413 when the byte cap is exceeded.
After cv2.imdecode(), validate decoded height, width, and total pixel count
against explicit limits before calling process_image_msae or _layer_to_rgba_png,
rejecting oversized or invalid images with the appropriate client error.
---
Outside diff comments:
In `@landing-page/scripts/tasks_encode.py`:
- Around line 99-100: Add con.rollback() to both exception handlers that
suppress database query errors in the affected task flow, before continuing with
fallback processing or releasing the connection. Keep the
get_latest_text_alignment() handler separate and do not add a duplicate rollback
there because that helper already performs it.
In `@landing-page/scripts/tests/test_encode_to_mei.py`:
- Around line 403-411: Update the assign_glyphs_to_staves call in the test to
pass allow_synthetic_lines=True, then assert the recovered stave has the
expected number of lines before computing spacings; retain the existing 20px
spacing assertion so it cannot pass on an empty line_ys list.
In `@landing-page/src/components/shared/FolioSelect.tsx`:
- Around line 15-21: Update FolioSelect’s custom-input change handling so
invalid or unrecognized folio text cannot remain displayed while the parent
value retains the previous valid selection. When customInput is invalid,
propagate an empty or other explicit falsy invalid value through onChange (or
validate before submission), while preserving normal propagation for valid
custom and canonical folio selections.
---
Duplicate comments:
In `@paco-classifier-service/main.py`:
- Around line 114-131: Add a process-wide bounded semaphore around the model
invocation in classify, limiting concurrent process_image_msae calls to the
capacity validated against memory usage and engine thread safety. Acquire it
without blocking indefinitely; when unavailable, return an HTTP 503 or 429
response, while preserving the existing image decoding and successful
classification flow.
---
Nitpick comments:
In `@landing-page/scripts/inference_api.py`:
- Around line 198-201: Rename the unused con binding in the db_cursor() context
manager unpacking to an underscore, matching the existing convention for
intentionally unused values and resolving Ruff RUF059.
In `@landing-page/scripts/job_store.py`:
- Around line 83-88: Update the database schema or migration used by the job
store to add a partial index on jobs covering project_id and created_at DESC,
restricted to rows whose status is pending or running. Ensure it supports both
active-job queries in the function, including the ORDER BY created_at DESC
lookup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b68ad1f-623a-473b-861d-85c62138256a
📒 Files selected for processing (45)
.github/workflows/build-images.yml.github/workflows/tests.yml.gitignoredev.shdocker-compose.ymlk8s/paco-classifier-service.yamllanding-page/scripts/auth_api.pylanding-page/scripts/batch_api.pylanding-page/scripts/encode_to_mei.pylanding-page/scripts/inference_api.pylanding-page/scripts/job_store.pylanding-page/scripts/mei_api.pylanding-page/scripts/staffline_adapter.pylanding-page/scripts/tasks_encode.pylanding-page/scripts/tasks_predict.pylanding-page/scripts/tests/test_bbox_pipeline_integrity.pylanding-page/scripts/tests/test_encode_to_mei.pylanding-page/scripts/tests/test_resolve_hints_staleness.pylanding-page/scripts/tests/test_staffline_adapter.pylanding-page/scripts/yolo_inference.pylanding-page/src/components/AppRouter.tsxlanding-page/src/components/project/BatchFolioReviewModal.tsxlanding-page/src/components/project/BatchTab.tsxlanding-page/src/components/project/CantusSourcePanel.tsxlanding-page/src/components/project/EditFolioModel.tsxlanding-page/src/components/project/ImageTab.tsxlanding-page/src/components/project/MeiViewerModal.tsxlanding-page/src/components/project/ModelTab.tsxlanding-page/src/components/project/ProjectDetail.tsxlanding-page/src/components/project/RhythmChart.tsxlanding-page/src/components/project/StafflineViewerModal.tsxlanding-page/src/components/project/TextAlignmentViewerModal.tsxlanding-page/src/components/project/TextAlignmentsTab.tsxlanding-page/src/components/shared/FolioSelect.tsxlanding-page/src/components/workflow/InteractiveClassifier.tsxlanding-page/src/components/workflow/ProcessingPage.tsxlanding-page/src/hooks/useActiveJobWatcher.tslanding-page/src/hooks/useEncodingFlow.tslanding-page/src/hooks/useTextFindingSettings.tslanding-page/src/lib/activeJobs.tslanding-page/src/lib/apiFetch.tslanding-page/src/utils/folio.tspaco-classifier-service/Dockerfilepaco-classifier-service/main.pypaco-classifier-service/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (27)
- .gitignore
- landing-page/src/components/project/TextAlignmentsTab.tsx
- paco-classifier-service/Dockerfile
- landing-page/src/components/project/RhythmChart.tsx
- landing-page/src/hooks/useTextFindingSettings.ts
- landing-page/src/components/project/CantusSourcePanel.tsx
- landing-page/src/hooks/useActiveJobWatcher.ts
- landing-page/src/components/project/BatchFolioReviewModal.tsx
- .github/workflows/build-images.yml
- landing-page/src/components/project/EditFolioModel.tsx
- landing-page/src/components/project/TextAlignmentViewerModal.tsx
- landing-page/scripts/staffline_adapter.py
- landing-page/scripts/yolo_inference.py
- landing-page/src/components/project/BatchTab.tsx
- landing-page/scripts/batch_api.py
- landing-page/src/components/project/StafflineViewerModal.tsx
- dev.sh
- landing-page/src/components/workflow/ProcessingPage.tsx
- landing-page/scripts/tests/test_staffline_adapter.py
- landing-page/src/hooks/useEncodingFlow.ts
- landing-page/src/components/workflow/InteractiveClassifier.tsx
- landing-page/src/components/project/ModelTab.tsx
- landing-page/src/components/project/MeiViewerModal.tsx
- landing-page/scripts/tasks_predict.py
- landing-page/src/components/project/ProjectDetail.tsx
- landing-page/src/components/AppRouter.tsx
- landing-page/src/components/project/ImageTab.tsx
Comment on lines
+83
to
+93
| cur.execute( | ||
| "SELECT job_id, kind FROM jobs WHERE project_id=%s" | ||
| " AND status IN ('pending','running')" | ||
| " ORDER BY created_at DESC LIMIT 1", | ||
| (project_id,), | ||
| ) | ||
| row = cur.fetchone() | ||
| active = {"job_id": row[0], "kind": row[1]} if row else None | ||
| if active is not None and active["kind"] not in (allowed_kinds or {kind}): | ||
| con.commit() # nothing written yet; just releases the advisory lock | ||
| return None, False, active |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check every active job, not only the newest one.
The active-job query uses ORDER BY created_at DESC LIMIT 1. It inspects only the most recent pending or running job. If two jobs are active at once and the newest one has an allowed kind, a conflicting older job stays undetected, and the insert proceeds.
This function prevents new overlaps, but rows created before this change (or by any other writer to jobs) can still produce that state. Select the newest active job whose kind is not allowed, so the check does not depend on ordering.
🛠️ Proposed change
+ kinds = tuple(allowed_kinds or {kind})
cur.execute(
"SELECT job_id, kind FROM jobs WHERE project_id=%s"
" AND status IN ('pending','running')"
+ " AND kind <> ALL(%s)"
" ORDER BY created_at DESC LIMIT 1",
- (project_id,),
+ (project_id, list(kinds)),
)
row = cur.fetchone()
active = {"job_id": row[0], "kind": row[1]} if row else None
- if active is not None and active["kind"] not in (allowed_kinds or {kind}):
+ if active is not None:
con.commit() # nothing written yet; just releases the advisory lock
return None, False, active📝 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.
Suggested change
| cur.execute( | |
| "SELECT job_id, kind FROM jobs WHERE project_id=%s" | |
| " AND status IN ('pending','running')" | |
| " ORDER BY created_at DESC LIMIT 1", | |
| (project_id,), | |
| ) | |
| row = cur.fetchone() | |
| active = {"job_id": row[0], "kind": row[1]} if row else None | |
| if active is not None and active["kind"] not in (allowed_kinds or {kind}): | |
| con.commit() # nothing written yet; just releases the advisory lock | |
| return None, False, active | |
| kinds = tuple(allowed_kinds or {kind}) | |
| cur.execute( | |
| "SELECT job_id, kind FROM jobs WHERE project_id=%s" | |
| " AND status IN ('pending','running')" | |
| " AND kind <> ALL(%s)" | |
| " ORDER BY created_at DESC LIMIT 1", | |
| (project_id, list(kinds)), | |
| ) | |
| row = cur.fetchone() | |
| active = {"job_id": row[0], "kind": row[1]} if row else None | |
| if active is not None: | |
| con.commit() # nothing written yet; just releases the advisory lock | |
| return None, False, active |
🤖 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 `@landing-page/scripts/job_store.py` around lines 83 - 93, Update the
active-job query in the surrounding job-store function to filter out allowed
kinds in SQL and select the newest remaining conflicting active job, rather than
applying ORDER BY/LIMIT before checking the fetched row. Preserve the existing
None result when no conflicting job exists and return the conflicting job
metadata unchanged.
Comment on lines
31
to
+39
| // Start in custom mode if we're editing a folio that's already off-canonical, | ||
| // so re-opening a picker on a phantom-tagged image shows the typed value, | ||
| // not a blank dropdown that appears to have lost it. | ||
| const [customMode, setCustomMode] = useState(() => !!value && !options.includes(value)); | ||
| const [customInput, setCustomInput] = useState(() => (customMode ? value : "")); | ||
| const [customMode, setCustomMode] = useState( | ||
| () => !!value && !options.includes(value), | ||
| ); | ||
| const [customInput, setCustomInput] = useState(() => | ||
| customMode ? value : "", | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target file and relevant test files without running repo code.
fd -a 'FolioSelect\.(tsx|test\.tsx|spec\.tsx)$' . || true
if [ -f landing-page/src/components/shared/FolioSelect.tsx ]; then
echo '--- FolioSelect.tsx outline ---'
ast-grep outline landing-page/src/components/shared/FolioSelect.tsx --view expanded || true
echo '--- FolioSelect.tsx ---'
cat -n landing-page/src/components/shared/FolioSelect.tsx
fi
echo '--- Related tests/usages ---'
rg -n "FolioSelect|customMode|customInput|CUSTOM_VALUE" landing-page/src -S || trueRepository: DDMAL/mothra
Length of output: 7250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Static behavioural model of the state-transition described in FolioSelect.tsx.
python3 - <<'PY'
from enum import Enum
class V(Enum):
CANON1 = "canon1"
CUSTOM = "custom"
EMPTY = None
CUSTOM_VALUE = "CUSTOM_VALUE"
class Picker:
def __init__(self, initial_value, initial_options, prev_custom_mode=False, prev_custom_input=""):
self.props_value = initial_value
self.props_options = list(initial_options)
# Mirrors useState(initialValue): runs once during mount.
self.state_custom_mode = bool(initial_value) and initial_value not in initial_options
if prev_custom_mode and prev_custom_input != "":
# If caller simulates a prior custom mode transition before first external change.
self.state_custom_mode = prev_custom_mode
self.state_custom_input = prev_custom_input
else:
self.state_custom_input = self.props_value if self.state_custom_mode else ""
def update_props(self, value, options):
self.props_value = value
self.props_options = list(options)
def value(self):
return self.state_val if hasattr(self, "state_val") else self.props_value
def custom_input(self):
return self.state_custom_input
def render_mode(self):
return "custom" if self.state_custom_mode else "select"
def set_state(self, state_custom_mode, state_custom_input=None, state_val=None):
self.state_custom_mode = state_custom_mode
if state_custom_input is not None:
self.state_custom_input = state_custom_input
if state_val is not None:
self.state_val = state_val
def select_custom(self):
# Mirrors the custom selection path: custom mode and custom input are set from value.
self.state_custom_mode = True
self.state_custom_input = self.value()
def reset(self):
# Mirrors clearing value: customMode state is recomputed once at mount and not used.
self.props_value = None
# if the component sets customInput when customMode AND reset happens, state_custom_input persists stale custom input.
self.state_custom_input = "" if self.state_custom_mode and self.props_value is None else self.state_custom_input
cases = [
("mount_custom_select_no_options", V.CUSTOM, ["canon1"]),
("external_value_shift_custom_canonical", V.CUSTOM, ["canon1"], True, V.CANON1, ["canon1"]),
("reset_with_stale_custom_input", V.CUSTOM, ["canon1"], True, None, ["canon1"]),
("new_custom_value_same_options", V.CANON1, ["canon1", "canon2"], False, "new", ["canon1", "canon2"]),
]
for case in cases:
if len(case) == 3:
name, value, options = case
p = Picker(value, options)
else:
name, start_value, options, selected_custom, new_value, new_options = case
p = Picker(value=value, initial_options=options, prev_custom_mode=selected_custom, prev_custom_input=value if selected_custom else "")
p.update_props(new_value, new_options)
print(f"\nCASE {name}")
print(f"props_value={p.props_value!r} props_options={p.props_options}")
print(f"render_mode={p.render_mode()} custom_input={p.custom_input()!r} value={p.value()!r}")
if hasattr(p, "state_custom_input"):
print(f"state_custom_mode={p.state_custom_mode} state_custom_input={p.state_custom_input!r}")
PYRepository: DDMAL/mothra
Length of output: 527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the exact relevant render block and onChange handlers without running repo code,
# because the previous command printed the whole file and we still need line-accurate text for rewrite.
if [ -f landing-page/src/components/shared/FolioSelect.tsx ]; then
sed -n '1,130p' landing-page/src/components/shared/FolioSelect.tsx | cat -n
fi
# Locate test files with precise names from listing result.
echo '--- Candidate test excerpts ---'
test_file="$(fd 'FolioSelect\.(test|spec)\.tsx$' . | head -n 1 || true)"
if [ -n "${test_file:-}" ]; then
wc -l "$test_file"
sed -n '1,220p' "$test_file" | cat -n
fiRepository: DDMAL/mothra
Length of output: 3921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
class Picker:
def __init__(
self,
initial_value,
initial_options,
prev_custom_mode=False,
prev_custom_input="",
):
self.props_value = initial_value
self.props_options = list(initial_options)
self.state_custom_mode = bool(initial_value) and initial_value not in initial_options
if prev_custom_mode and prev_custom_input != "":
self.state_custom_mode = prev_custom_mode
self.state_custom_input = prev_custom_input
else:
self.state_custom_input = self.props_value if self.state_custom_mode else ""
self.state_val = initial_value
def update_props(self, value, options):
self.props_value = value
self.props_options = list(options)
def select_custom(self):
self.state_custom_mode = True
self.state_custom_input = self.state_val
self.state_val = "__custom__"
def choose_select_option(self, option):
self.state_custom_mode = False
self.state_custom_input = ""
self.state_val = option
print("\nCASE external_value_shift_with_prior_custom_transition")
p = Picker("003r", ["canon1"], prev_custom_mode=True, prev_custom_input="003r")
p.update_props("002r", ["canon1", "002r"])
print("props_value=", p.props_value, "props_options=", p.props_options)
print("render_mode =", "custom" if p.state_custom_mode else "select")
print("input_value =", repr(p.state_custom_input), "select_value =", repr(p.state_val))
print("stale_custom_displayed =", p.state_custom_input != p.props_value or p.state_val != p.props_value)
print("\nCASE reset_after_custom_mode")
p = Picker("003r", ["canon1"], prev_custom_mode=True, prev_custom_input="003r")
p.update_props("", ["canon1"])
print("render_mode =", "custom" if p.state_custom_mode else "select")
print("input_value =", repr(p.state_custom_input))
print("stale_cleared_display =", p.state_custom_input == "003r")
print("\nCASE new_custom_value_same_options_after_select")
p = Picker("", ["canon1"])
p.choose_select_option("canon1")
p.select_custom()
p.update_props("003r", ["canon1", "003r"])
print("props_value=", p.props_value, "props_options=", p.props_options)
print("render_mode =", "custom" if p.state_custom_mode else "select")
print("input_value =", repr(p.state_custom_input), "select_value =", repr(p.state_val))
print("missing_provided_canonical_option =", "003r" not in p.props_options)
PY
# Inspect caller contexts for FolioSelect to identify real propagation paths.
for f in landing-page/src/components/project/CantusSourcePanel.tsx landing-page/src/components/project/EditFolioModel.tsx; do
if [ -f "$f" ]; then
echo "--- $f relevant FolioSelect blocks ---"
rg -n -C 8 "FolioSelect" "$f" -S
fi
doneRepository: DDMAL/mothra
Length of output: 387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
class Picker:
def __init__(
self,
initial_value,
initial_options,
prev_custom_mode=False,
prev_custom_input="",
):
self.props_value = initial_value
self.props_options = list(initial_options)
self.state_custom_mode = bool(initial_value) and initial_value not in initial_options
if prev_custom_mode and prev_custom_input != "":
self.state_custom_mode = prev_custom_mode
self.state_custom_input = prev_custom_input
else:
self.state_custom_input = self.props_value if self.state_custom_mode else ""
self.state_val = initial_value
def update_props(self, value, options):
self.props_value = value
self.props_options = list(options)
def select_custom(self):
self.state_custom_mode = True
self.state_custom_input = self.state_val
self.state_val = "__custom__"
def choose_select_option(self, option):
self.state_custom_mode = False
self.state_custom_input = ""
self.state_val = option
def toggle_back_to_list(self):
self.state_custom_mode = False
self.state_custom_input = ""
self.state_val = ""
def typed_input_change(self, text):
self.state_custom_input = text
self.state_val = text
def render_mode(self):
return "custom" if self.state_custom_mode else "select"
cases = {
"external_value_shift_with_prior_custom_transition": lambda: _case_prior_custom("002r", ["canon1", "002r"]),
"reset_after_custom_mode": lambda: _case_prior_custom("", ["canon1"]),
"new_custom_value_after_select_custom": lambda: _case_after_select_custom("003r", ["canon1", "003r"]),
"reset_after_select_custom": lambda: _case_after_select_custom("", ["canon1"]),
"type_after_select_custom_to_new_canonical_with_option": lambda: _case_after_select_custom("003r", ["canon1", "003r"]),
}
def _case_prior_custom(new_value, new_options):
p = Picker("003r", ["canon1"], prev_custom_mode=True, prev_custom_input="003r")
p.update_props(new_value, new_options)
return p
def _case_after_select_custom(new_value, new_options):
p = Picker("", ["canon1"])
p.choose_select_option("canon1")
p.select_custom()
p.update_props(new_value, new_options)
return p
for name, make in cases.items():
p = make()
print(f"\nCASE {name}")
print("props_value=", p.props_value, "props_options=", p.props_options)
print("render_mode =", p.render_mode())
val_display = repr(p.state_val) if p.render_mode() == "select" else repr(p.state_custom_input)
print("displayed_value =", val_display)
print("missing_provided_option =", p.props_value not in p.props_options)
print("stale_display =", val_display != repr(p.props_value))
echo "--- FolioSelect callers relevant blocks ---"
for f in landing-page/src/components/project/CantusSourcePanel.tsx landing-page/src/components/project/EditFolioModel.tsx; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -C 8 "FolioSelect" "$f" -S || true
fi
done
PYRepository: DDMAL/mothra
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
class Picker:
def __init__(
self,
initial_value,
initial_options,
prev_custom_mode=False,
prev_custom_input="",
):
self.props_value = initial_value
self.props_options = list(initial_options)
self.state_custom_mode = bool(initial_value) and initial_value not in initial_options
if prev_custom_mode and prev_custom_input != "":
self.state_custom_mode = prev_custom_mode
self.state_custom_input = prev_custom_input
else:
self.state_custom_input = self.props_value if self.state_custom_mode else ""
self.state_val = initial_value
def update_props(self, value, options):
self.props_value = value
self.props_options = list(options)
def select_custom(self):
self.state_custom_mode = True
self.state_custom_input = self.state_val
self.state_val = "__custom__"
def choose_select_option(self, option):
self.state_custom_mode = False
self.state_custom_input = ""
self.state_val = option
def toggle_back_to_list(self):
self.state_custom_mode = False
self.state_custom_input = ""
self.state_val = ""
def typed_input_change(self, text):
self.state_custom_input = text
self.state_val = text
def render_mode(self):
return "custom" if self.state_custom_mode else "select"
cases = {
"external_value_shift_with_prior_custom_transition": lambda: _case_prior_custom("002r", ["canon1", "002r"]),
"reset_after_custom_mode": lambda: _case_prior_custom("", ["canon1"]),
"new_custom_value_after_select_custom": lambda: _case_after_select_custom("003r", ["canon1", "003r"]),
"reset_after_select_custom": lambda: _case_after_select_custom("", ["canon1"]),
}
def _case_prior_custom(new_value, new_options):
p = Picker("003r", ["canon1"], prev_custom_mode=True, prev_custom_input="003r")
p.update_props(new_value, new_options)
return p
def _case_after_select_custom(new_value, new_options):
p = Picker("", ["canon1"])
p.choose_select_option("canon1")
p.select_custom()
p.update_props(new_value, new_options)
return p
for name, make in cases.items():
p = make()
print(f"\nCASE {name}")
print("props_value=", p.props_value, "props_options=", p.props_options)
print("render_mode =", p.render_mode())
def displayed():
if p.render_mode() == "select":
return repr(p.state_val)
return repr(p.state_custom_input)
print("displayed_value =", displayed())
print("missing_provided_option =", p.props_value not in p.props_options)
print("stale_display =", displayed() != repr(p.props_value))
PY
echo "--- FolioSelect callers relevant blocks ---"
for f in landing-page/src/components/project/CantusSourcePanel.tsx landing-page/src/components/project/EditFolioModel.tsx; do
if [ -f "$f" ]; then
echo "### $f"
rg -n -C 8 "FolioSelect" "$f" -S || true
fi
doneRepository: DDMAL/mothra
Length of output: 5061
Sync local picker state with custom-value updates.
When the parent re-renders value after the picker enters custom mode, customInput does not update because it is only derived from customMode at mount. This leaves the free-text input showing the old custom value. Set or reset the input whenever value changes in custom mode and keep the reset handled for cleared values.
🤖 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 `@landing-page/src/components/shared/FolioSelect.tsx` around lines 31 - 39,
Update the local state synchronization in FolioSelect so customInput tracks
subsequent value changes whenever customMode is active, rather than only
deriving its initial value at mount. Add the necessary value-dependent effect or
equivalent update, and reset customInput when the value is cleared while
preserving existing behavior for canonical values and mode transitions.
Comment on lines
+274
to
+278
| // fall back to today's positional behavior | ||
| sequence.push(nextCanonical); | ||
| isPhantom.push(false); | ||
| if (nextCanonical !== undefined) lastAccepted = nextCanonical; | ||
| c++; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not advance canonicalConsumed after the canonical list ends.
If nextCanonical is undefined, this branch consumes no canonical folio. The unconditional c++ makes canonicalConsumed exceed canonical.length for surplus uploads.
Increment c only when nextCanonical exists.
Proposed fix
sequence.push(nextCanonical);
isPhantom.push(false);
- if (nextCanonical !== undefined) lastAccepted = nextCanonical;
- c++;
+ if (nextCanonical !== undefined) {
+ lastAccepted = nextCanonical;
+ c++;
+ }📝 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.
Suggested change
| // fall back to today's positional behavior | |
| sequence.push(nextCanonical); | |
| isPhantom.push(false); | |
| if (nextCanonical !== undefined) lastAccepted = nextCanonical; | |
| c++; | |
| // fall back to today's positional behavior | |
| sequence.push(nextCanonical); | |
| isPhantom.push(false); | |
| if (nextCanonical !== undefined) { | |
| lastAccepted = nextCanonical; | |
| c++; | |
| } |
🤖 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 `@landing-page/src/utils/folio.ts` around lines 274 - 278, Update the fallback
branch that pushes nextCanonical so c (canonicalConsumed) increments only when
nextCanonical is defined; preserve sequence and isPhantom updates while
preventing surplus uploads from advancing beyond canonical.length.
Comment on lines
+97
to
+110
| @app.get("/health") | ||
| def health(): | ||
| """Liveness/readiness signal for Compose's healthcheck and k8s's probes | ||
| (see docker-compose.yml / k8s/paco-classifier-service.yaml). Deliberately | ||
| does NOT exercise recognition_engine.process_image_msae() -- it calls | ||
| tensorflow.keras.models.load_model() fresh on every single request (no | ||
| caching in the vendored submodule), so there is no persistent "model | ||
| warmed" state to probe for; the meaningful, cheap thing to confirm | ||
| instead is that this process is actually serving HTTP (not just that the | ||
| OS has a listener on the port, which a bare TCP probe can't tell apart | ||
| from an app that bound the port and then crashed) and that the weight | ||
| files this process already validated at import time are still there.""" | ||
| if not BACKGROUND_MODEL_PATH.exists() or not STAFFLINES_MODEL_PATH.exists(): | ||
| raise HTTPException(status_code=503, detail="staffline classifier weights missing") |
There was a problem hiding this comment.
Make /health validate classifier readiness.
health() returns 200 when both weight paths exist, but classify() loads the Keras models later through recognition_engine.process_image_msae(). A corrupt, incompatible, or unreadable weight file can therefore pass the Compose and Kubernetes readiness checks while every /classify call fails. tasks_predict.py then falls back to raw-page detection, so the new classifier can be silently bypassed.
Use separate liveness and readiness endpoints. Make readiness perform a one-time model-load validation or report an initialized engine state. This is the same readiness gap raised in the previous review, and it remains present.
This follows the supplied /health implementation and the Compose/Kubernetes probe contract.
Verification command
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'def health|load_model|process_image_msae|BACKGROUND_MODEL_PATH|STAFFLINES_MODEL_PATH' \
paco-classifier-service/main.py paco-classifier🤖 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 `@paco-classifier-service/main.py` around lines 97 - 110, Separate the current
health check into liveness and readiness endpoints, keeping liveness lightweight
while making readiness validate that both classifier models can load
successfully or use an explicitly initialized engine state. Update the Compose
and Kubernetes probe targets to use the readiness endpoint, and ensure
classify() relies on the same validated readiness state before calling
recognition_engine.process_image_msae().
Comment on lines
+114
to
+121
| @app.post("/classify") | ||
| def classify(image: UploadFile = File(...)): | ||
| # Plain `def`, not `async def`: FastAPI runs a sync endpoint in its | ||
| # threadpool automatically, so the blocking TensorFlow/OpenCV/PNG work | ||
| # below (process_image_msae, _layer_to_rgba_png) doesn't stall the event | ||
| # loop the way it would inside an `async def` with no `await` yield | ||
| # points. image.file is a SpooledTemporaryFile -- .read() is sync here. | ||
| data = image.file.read() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== locate main.py =="
fd -a 'main.py' . | sed 's#^\./##'
echo "== main outline =="
ast-grep outline paco-classifier-service/main.py 2>/dev/null || true
echo "== relevant main.py lines =="
nl -ba paco-classifier-service/main.py | sed -n '1,220p'
echo "== search for uploads/image limits =="
rg -n "MAX_UPLOAD|upload|max_upload|413|content-length|length|limit|imdecode|decode\(|RGBA|pixel|dimension|size" paco-classifier-service -S || true
echo "== k8s resource snippets =="
git ls-files | xargs rg -n "resources:|limits:|requests:|memory:|cpu:|paco-classifier-service|classify" -S || trueRepository: DDMAL/mothra
Length of output: 10064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== paco-classifier-service/main.py with line numbers =="
awk '{printf "%6d %s\n", NR, $0}' paco-classifier-service/main.py | sed -n '1,220p'
echo "== search upload/decode/limit symbols =="
rg -n "UploadFile|image\.file\.read|imdecode|imread|resize|numpy\.zeros|max_length|MAX_UPLOAD|413|content-length|Content-Length|length-limit|width|height|pixel" paco-classifier-service -S || true
echo "== Dockerfile / compose / resource files referencing classifier =="
rg -n "paco-classifier-service|paco_classifier|classifier|memory:|cpu:|limits:|requests:|classify" .github configs Dockerfile* docker-compose* deployment*.yaml deployment*.yml .yaml .yml -S 2>/dev/null || true
echo "== package requirements for opencv version hints =="
fd -i 'requirements.txt|Pipfile|pyproject.toml|environment.yml' . -a | xargs rg -n "opencv|tensorflow|fastapi|uvicorn|torch" -S 2>/dev/null || trueRepository: DDMAL/mothra
Length of output: 13073
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-compose classifier excerpt =="
awk '{printf "%6d %s\n", NR, $0}' docker-compose.yml | sed -n '30,85p'
echo "== k8s paco-classifier service excerpt =="
awk '{printf "%6d %s\n", NR, $0}' k8s/paco-classifier-service.yaml | sed -n '1,220p'
echo "== dockerfiles =="
git ls-files 'paco-classifier-service/Dockerfile' 'Dockerfile' | xargs -r awk '{printf "\n---- %s ----\n", FILENAME; NR=1} {printf "%6d %s\n", NR++, $0}'
echo "== deterministic OpenCV decode behavior probe =="
python3 - <<'PY'
import sys
try:
import cv2
import numpy as np
except Exception as e:
print(f"IMPORT_ERROR: {type(e).__name__}: {e}")
sys.exit(0)
sizes = [
(1024, 1024, b"\xff\xdb\x00C\x00"),
(2048, 2048, b"\xff\xd8\xff\xe0\x00\x10JFIF\x00"),
(4096, 4096, b"\x89PNG\r\n\x1a\n"),
]
for w, h, body in sizes:
decoded = cv2.imdecode(np.frombuffer(body, np.uint8), cv2.IMREAD_COLOR)
decoded_shape = decoded.shape[:2] if decoded is not None else None
if decoded is not None:
mem_bytes = decoded.size * decoded.dtype.itemsize
pixel_bytes = w * h * 3 if decoded.shape[:2] == (h, w) else None
print(f"input_bytes={len(body):d} -> decoded_shape={decoded_shape} cv2_mem_bytes={mem_bytes} -> original_bytes_est={pixel_bytes}")
else:
print(f"input_bytes={len(body):d} -> decoded=None")
PYRepository: DDMAL/mothra
Length of output: 8034
🌐 Web query:
OpenCV cv2.imdecode uncompressed 3-channel image pixel memory bytes
💡 Result:
When you use OpenCV's cv2.imdecode to decompress an image into a 3-channel (color) matrix, the resulting data is stored in memory as a dense, row-major array with a specific structure [1][2][3]. Key Memory Layout Details: 1. Channel Order: OpenCV stores color images in BGR (Blue, Green, Red) order, rather than the more common RGB order found in many other libraries [1][4]. 2. Interleaved Channels: The pixel data is interleaved. A 3-channel image does not store all blue values together, then all green, then all red. Instead, for each pixel, the bytes are stored sequentially as B-G-R, B-G-R, and so on [3]. 3. Row-Major and Contiguity: The image is stored in row-major order [3]. While data is typically stored as a contiguous block of memory, OpenCV matrices can sometimes include padding bytes at the end of each row to maintain memory alignment for performance [5][3]. 4. Accessing Data: Because of potential row padding, you should not rely on simple multiplication (e.g., width * height * 3) to locate a pixel [5]. Instead, you should use the matrix's step attribute [5][6]: - The address of a pixel at row y and column x is: data_ptr + (y * step) + (x * elemSize) [3]. - The specific byte for a channel c (where 0=B, 1=G, 2=R) within that pixel is accessed by adding c to the pixel's base address [3]. For standard 8-bit images (CV_8UC3), each channel occupies 1 byte, meaning each pixel consumes 3 bytes [7][3]. If the matrix is continuous (which can be checked using mat.isContinuous), the entire image data is a single contiguous block of size rows * cols * 3 bytes [3][8].
Citations:
- 1: https://docs.opencv.org/4.11.0/d4/da8/group__imgcodecs.html
- 2: https://docs.opencv.org/3.4.11/d3/d63/classcv_1_1Mat.html
- 3: https://web.stanford.edu/class/cs231m/lectures/lecture-4-opencv.pdf
- 4: https://www.exchangetuts.com/index.php/differences-between-pil-imageopen-and-cv2imdecode-1641327305305066
- 5: https://github.com/opencv/opencv/blob/4.x/modules/core/include/opencv2/core/mat.hpp
- 6: https://docs.opencv.org/4.10.0/d3/d63/classcv%5F1%5F1Mat.html
- 7: https://docs.opencv.org/4.3.0/d6/d6d/tutorial_mat_the_basic_image_container.html
- 8: https://stackoverflow.com/questions/37040787/opencv-in-memory-mat-representation
Bound image input before allocating pixels.
data = image.file.read() uploads the full multipart body, while cv2.imdecode() allocates a dense buffer from the decoded height, width, and byte depth. The POST /classify handler has no local input byte limit or decoded-pixel limit, so a reachable client or forwarded image can exceed the service’s 4 GiB container memory limit before decoding fails. Read a fixed byte cap and reject 413, then validate decoded dimensions/pixel count before running process_image_msae() or encoding output PNGs.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 115-115: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 `@paco-classifier-service/main.py` around lines 114 - 121, Update the classify
handler to enforce a maximum upload size while reading image.file, returning
HTTP 413 when the byte cap is exceeded. After cv2.imdecode(), validate decoded
height, width, and total pixel count against explicit limits before calling
process_image_msae or _layer_to_rgba_png, rejecting oversized or invalid images
with the appropriate client error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related pieces of work:
- Syllable text was landing on the wrong row, or blank, when a row's neumes got fragmented across internal stave buckets, or when adjacent staves' syllables cross-assigned. mothra-text's syllable order/text is now the source of truth Neon cross-references against and corrects toward.
- Notes were rendering visibly off their real staff position. Root cause was two-fold: unreliable line-spacing estimates for staves the detector missed or under/over-detected, and a mismatch between the stave zone's height (which Verovio derives its rendered note spacing from) and the real measured line spacing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests