fix: validate retained visual locator regions - #324
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
@opencode-agent review exact current HEAD a4627d6e5e65f3f25bdba9f316cdabb2d933fb0b. Review locator-bound validation and the stacked #320 visual evidence contract; publish a formal review. |
|
Supersede the preceding review request: @opencode-agent review exact current HEAD a4627d6. Review locator-bound validation and the stacked #320 visual evidence contract; publish a formal review. |
|
Supersede all preceding review requests: @opencode-agent review exact current HEAD 3a335ad. Verify the retained visual locator contract, ADR 0104, and the 100% branch-covered fallback tests; publish a formal review. |
|
Supersede the preceding review request: @opencode-agent review exact current HEAD 71f0940. The malformed locator fallback is now covered by the full regression () and normalization branch coverage (). |
|
Supersede the preceding review request: @opencode-agent review exact current HEAD 71f0940. The malformed locator fallback is covered by the full regression: 740 passed, 16 skipped. Normalization branch coverage is 100 percent. Publish a formal review for this SHA. |
|
Supersede prior review targets: current exact HEAD is |
|
Supersede all preceding review requests: @opencode-agent review exact current HEAD fdd62a6. The product baseline now records this exact head, 740 Python tests pass with 16 skips, and changed normalization branch coverage is 100 percent. Publish a formal review for this SHA. |
|
Current exact HEAD is |
|
Review exact current HEAD fdd62a6. Visual locator validation now rejects malformed/nonfinite/out-of-bounds regions and falls back to bounded source-derived regions; local evidence is 740 Python tests passed, 16 skipped, changed normalizer branch coverage 100 percent, frontend checks and Storybook build passed. Please publish a formal independent review for this exact SHA. |
|
Exact-head review request: current HEAD is |
…ions' into HEAD # Conflicts: # docs/product-technical-gap-baseline.md # lineageweave/post_content_normalization.py
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed the valid review finding on exact current head b1d1b10. Removed the orphaned duplicate "Integration status: PR #320" note from the PR #324 section of docs/product-technical-gap-baseline.md. The note now appears exactly once under the PR #320 partial visual-region checkpoint. Validation:
Please review this exact current HEAD; predecessor evidence is stale. |
|
Exact current head is 7a49d3e. Follow-up to the prior review fix:
Validation:
Please review only the exact current head 7a49d3e; earlier evidence is superseded. |
|
Reviewed and repaired against exact current head
The PR remains unmergeable until the required independent review and terminal Checks are present; no bypass used. |
…ions' into repair/pr324-duplicate-gap-status
…ions' into repair/pr324-duplicate-gap-status # Conflicts: # lineageweave/post_content_normalization.py # tests/test_post_content_normalization.py
…ions' into repair/pr324-duplicate-gap-status
Current-head review evidence at
|
…ions' into codex/review-pr324
| bound to it, so this is not a merge or release claim. | ||
| ## Image locator and buyer table checkpoint: 2026-08-20 |
There was a problem hiding this comment.
🟡 Missing blank line before heading
The new refresh paragraph runs directly into the ## Image locator and buyer table checkpoint heading with no blank line between them. This breaks the blank-line-before-heading convention used everywhere else in the file and can prevent strict markdown renderers from treating the line as a heading.
| bound to it, so this is not a merge or release claim. | |
| ## Image locator and buyer table checkpoint: 2026-08-20 | |
| bound to it, so this is not a merge or release claim. | |
| ## Image locator and buyer table checkpoint: 2026-08-20 |
Was this helpful? React with 👍 or 👎 to provide feedback.
* fix: keep internal image instructions out of buyer evidence * fix: protect persisted image region captions * fix: preserve legitimate Korean image captions * docs: allocate unique buyer image ADR number * fix: reject invalid vision region response types * fix: block internal vision destinations * fix: keep provider failures out of buyer errors * feat: trace post processing and Valkey sessions (#345) * feat: trace post processing and valkey sessions * docs: align orchestrator runtime pin * docs: codify telemetry boundary * fix: normalize OTLP trace endpoint * chore: pin orchestrator telemetry fix * fix: keep provider details out of ingestion ledger * fix: propagate W3C trace context to gateway
260e919
into
codex/preserve-partial-image-regions
* fix: retain partial image regions * docs: record partial image region gap * test: cover partial parent vision failure * fix: safely handle malformed region locator output * style: use deferred region result annotation * docs: reconcile partial-region evidence counts * fix: validate retained visual locator regions (#324) * fix: reject unbounded visual locator regions * docs: refresh exact-head product gap audit * test: cover visual region fallback branches * fix: fall back from malformed visual locator output * docs: refresh visual locator exact-head evidence * test: keep invalid locator fallback coordinate-free * docs: keep PR 320 status under its checkpoint * docs: refresh PR 324 exact-head evidence * fix: safely handle malformed region locator output * fix: keep internal image instructions out of buyer evidence (#329) * fix: keep internal image instructions out of buyer evidence * fix: protect persisted image region captions * fix: preserve legitimate Korean image captions * docs: allocate unique buyer image ADR number * fix: reject invalid vision region response types * fix: block internal vision destinations * fix: keep provider failures out of buyer errors * feat: trace post processing and Valkey sessions (#345) * feat: trace post processing and valkey sessions * docs: align orchestrator runtime pin * docs: codify telemetry boundary * fix: normalize OTLP trace endpoint * chore: pin orchestrator telemetry fix * fix: keep provider details out of ingestion ledger * fix: propagate W3C trace context to gateway
* fix: separate source whitespace from explicit structure * docs: record source indentation buyer gap * fix: expose unexpected content channel defects * fix: expose recoverable content batch failures * docs: normalize gap baseline headings * fix: retain partial visual regions (#320) * fix: retain partial image regions * docs: record partial image region gap * test: cover partial parent vision failure * fix: safely handle malformed region locator output * style: use deferred region result annotation * docs: reconcile partial-region evidence counts * fix: validate retained visual locator regions (#324) * fix: reject unbounded visual locator regions * docs: refresh exact-head product gap audit * test: cover visual region fallback branches * fix: fall back from malformed visual locator output * docs: refresh visual locator exact-head evidence * test: keep invalid locator fallback coordinate-free * docs: keep PR 320 status under its checkpoint * docs: refresh PR 324 exact-head evidence * fix: safely handle malformed region locator output * fix: keep internal image instructions out of buyer evidence (#329) * fix: keep internal image instructions out of buyer evidence * fix: protect persisted image region captions * fix: preserve legitimate Korean image captions * docs: allocate unique buyer image ADR number * fix: reject invalid vision region response types * fix: block internal vision destinations * fix: keep provider failures out of buyer errors * feat: trace post processing and Valkey sessions (#345) * feat: trace post processing and valkey sessions * docs: align orchestrator runtime pin * docs: codify telemetry boundary * fix: normalize OTLP trace endpoint * chore: pin orchestrator telemetry fix * fix: keep provider details out of ingestion ledger * fix: propagate W3C trace context to gateway
| except Exception as exc: # noqa: BLE001 - provider boundary is fail-closed. | ||
| raise HTTPException( | ||
| status.HTTP_503_SERVICE_UNAVAILABLE, | ||
| "Post chat is unavailable: contextual-orchestrator returned no complete evidence object", | ||
| ) from exc |
There was a problem hiding this comment.
🟡 Duplicate unreachable exception handler
Two identical except Exception as exc: blocks now follow the same try (backend/app/main.py:2655-2664). The second can never execute. It is dead copy-paste code, and flake8-bugbear's duplicate-try-block rule can fail on it since only BLE001 is suppressed.
| except Exception as exc: # noqa: BLE001 - provider boundary is fail-closed. | |
| raise HTTPException( | |
| status.HTTP_503_SERVICE_UNAVAILABLE, | |
| "Post chat is unavailable: contextual-orchestrator returned no complete evidence object", | |
| ) from exc | |
| cited_ids = list(answer.cited_post_ids) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| with traced( | ||
| "lineageweave.valkey.analysis_outbox_xread", | ||
| { | ||
| "db.system": "redis", | ||
| "db.operation.name": "xread", | ||
| "lineageweave.stream.kind": "analysis_outbox", | ||
| }, | ||
| ): | ||
| batches = await client.xread({OUTBOX_STREAM_KEY: last_id}, count=10, block=1000) |
There was a problem hiding this comment.
🔍 Idle blocking reads emit a span every cycle
The worker loops forever calling client.xread(..., block=1000), now wrapped in traced. With an OTLP endpoint configured this produces a new ~1s span roughly once per second per worker even while the stream is idle. No-op when telemetry is off; verify the emitted volume is acceptable when it is on.
Was this helpful? React with 👍 or 👎 to provide feedback.
| raise TypeError("vision region response was not text JSON") | ||
| fenced = re.sub(r"^\s*```(?:json)?\s*|\s*```\s*$", "", content, flags=re.IGNORECASE) | ||
| document = json.loads(fenced) | ||
| if not isinstance(document, dict): | ||
| raise ValueError("vision region response had no regions list") | ||
| raise TypeError("vision region response had no regions list") |
There was a problem hiding this comment.
📝 Info: locate_regions exception-type change is caller-safe
Switching to TypeError for non-string content and non-dict document does not change behavior: the only production caller, _describe_image_chunk (lineageweave/post_content_normalization.py:187-191), catches Exception and falls back to whole-image evidence. The sibling 'no regions list' path still raises ValueError, so identical-sounding messages now carry different exception types.
Was this helpful? React with 👍 or 👎 to provide feedback.
| global _CONFIGURED | ||
| if _CONFIGURED or os.getenv("OTEL_SDK_DISABLED", "").lower() == "true": | ||
| return | ||
| _CONFIGURED = True | ||
| endpoint = os.getenv("OTEL_EXPORTER_OTLP_ENDPOINT", "").strip() | ||
| if trace is None or not endpoint: | ||
| return | ||
| try: | ||
| from opentelemetry.exporter.otlp.proto.http.trace_exporter import ( | ||
| OTLPSpanExporter, | ||
| ) | ||
| from opentelemetry.sdk.resources import Resource | ||
| from opentelemetry.sdk.trace import TracerProvider | ||
| from opentelemetry.sdk.trace.export import BatchSpanProcessor | ||
| except ImportError: # pragma: no cover - guarded by the runtime extra | ||
| _LOGGER.warning("OpenTelemetry SDK/exporter is unavailable") | ||
| return | ||
|
|
||
| resource = Resource.create({ | ||
| "service.name": os.getenv("OTEL_SERVICE_NAME", service_name), | ||
| "service.namespace": "contextualwisdomlab", | ||
| }) | ||
| provider = TracerProvider(resource=resource) | ||
| provider.add_span_processor( | ||
| BatchSpanProcessor( | ||
| OTLPSpanExporter(endpoint=_otlp_trace_endpoint(endpoint)) | ||
| ) | ||
| ) | ||
| trace.set_tracer_provider(provider) |
There was a problem hiding this comment.
📝 Info: _CONFIGURED set before setup succeeds
configure_telemetry sets the module global _CONFIGURED = True before verifying the endpoint or importing the SDK/exporter. If the SDK import fails, no provider is installed yet the flag stays set, so any later retry is a no-op. Fine for the single startup call, but blocks reconfiguration if reused.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
out-of-image bounds.
from fix: retain partial visual regions #320.
never persist a fabricated full-image region.
Dependency
0f8bc28f116dc4c6e3a04c01d042de53c51e9173.8bcfea8d91810323a50dff89ba0d2e74170928e2after an ordinary parentrestack.
Review finding addressed
#320's ADR requires valid bounded regions, but its implementation passed
unbounded locator output to crop/persistence. This follow-up enforces that
boundary before region analysis.
Verification
git diff --checkandactionlint: passed.uv run --locked --extra dev --extra backend python -m pytest -q:742 passed, 16 skipped, 4 warnings.48 passed.136tests, production build, and Storybookbuild: passed.
No real records, identifiers, or provider credentials are included.