Skip to content

fix: retain partial visual regions - #320

Merged
seonghobae merged 13 commits into
codex/normalize-source-indent-semanticsfrom
codex/preserve-partial-image-regions
Aug 21, 2026
Merged

fix: retain partial visual regions#320
seonghobae merged 13 commits into
codex/normalize-source-indent-semanticsfrom
codex/preserve-partial-image-regions

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Buyer-visible gap

A VISION locator that returned valid salient panels but not full-image coverage
was replaced by one full-image region. Panel coordinates, panel-level OCR, and
panel search attribution were therefore lost.

Change

  • Retain every valid bounded partial region.
  • Analyze the parent image once when locator coverage is partial, preserving
    evidence outside the panels.
  • Keep parent-image evidence for empty, invalid, or single full-image locator
    output without inventing a region coordinate.
  • Record the contract in ADR 0104 and add regressions asserting panel
    retention, parent analysis, and no fabricated full-image region.

Verification

  • Current parent fix: keep visual whitespace out of authoritative structure #319:
    e84994e9145d3f7a1a7849e94c91de02cccf62fc.
  • Current head:
    0f8bc28f116dc4c6e3a04c01d042de53c51e9173 after an ordinary parent
    restack.
  • uv run --locked --extra dev --extra backend python -m pytest -q
    734 passed, 16 skipped, 4 warnings.
  • Focused image/normalization tests — 13 passed.
  • Frontend lint, TypeScript build, 136 tests, production build, Storybook,
    actionlint, and git diff --check passed.

No real records, identifiers, or provider credentials are included. This PR
is stacked on #319, which is stacked on #317.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review exact current HEAD 34a0c98. Inspect partial visual-region retention, parent-image fallback, region status persistence, and current required Checks. Report actionable findings only; do not approve, bypass, or use stale parent evidence.

@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f42d58a-2474-4335-a2fc-0cdee55ad4a0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review exact current HEAD 799671a. Re-check partial visual-region retention, parent-image fallback, region status persistence, and current required Checks. Prior-head evidence is stale; report actionable findings only and do not approve or bypass.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review exact current HEAD 41d164c5c0c312b66982f8cf8c4c14f16ab7f80f. Re-check the partial visual-region and parent-failure paths plus current required Checks; prior-head evidence is stale. Report findings only; do not approve or bypass.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@opencode-agent review exact current HEAD 41d164c. Re-check the partial visual-region and parent-failure paths plus current required Checks; prior-head evidence is stale. Report findings only; do not approve or bypass.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Review exact current HEAD 41d164c. This is the dependency base for the visual-region validation stack; its required Full test suite and Frontend lint, test, build checks are green. Please publish a formal independent review for this exact SHA and confirm the base-chain relationship before merge.

…mantics' into codex/preserve-partial-image-regions

# Conflicts:
#	docs/product-technical-gap-baseline.md
#	lineageweave/post_content_normalization.py
#	tests/test_post_content_normalization.py

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@seonghobae

Copy link
Copy Markdown
Contributor Author

Review exact current HEAD 0f8bc28f116dc4c6e3a04c01d042de53c51e9173. Re-check partial visual-region retention, parent-image fallback, region status persistence, and the current required Checks; prior-head evidence is stale. Please publish a formal independent review for this exact SHA and report actionable findings only.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Reviewed and repaired against exact current head 91a95a976c406a6e9d96c943b74d68e6a1b5cdc7 (including concurrent upstream synchronization commits):

  • Partial locator regions remain independently described and the original parent image is still described for uncovered content.
  • A malformed non-iterable locator result now falls back to parent-image evidence instead of incorrectly failing the entire image.
  • Added a regression test for that provider-boundary case.
  • Focused verification: uv run --locked --extra dev --extra backend python -m pytest -q tests/test_post_content_normalization.py tests/test_image_content.py tests/test_post_content_persistence_edges.py51 passed.
  • git diff --check passes.

PR #324 remains the stacked follow-up for finite/in-bounds region validation. This PR remains blocked pending independent formal review and terminal Checks; no bypass used.

@seonghobae
seonghobae enabled auto-merge (squash) August 21, 2026 05:04
@seonghobae seonghobae added the bug Something isn't working label Aug 21, 2026 — with ChatGPT Codex Connector
…mantics' into codex/review-pr320

# Conflicts:
#	docs/adr/0104-partial-visual-regions-retain-parent-evidence.md
#	docs/product-technical-gap-baseline.md
#	lineageweave/post_content_normalization.py
#	tests/test_post_content_normalization.py
devin-ai-integration[bot]

This comment was marked as resolved.

* 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
@seonghobae
seonghobae merged commit 365f253 into codex/normalize-source-indent-semantics Aug 21, 2026
3 of 4 checks passed
seonghobae added a commit that referenced this pull request Aug 21, 2026
* 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

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 new potential issues.

Open in Devin Review

Comment thread backend/app/main.py
Comment on lines +2660 to +2664
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Duplicate unreachable exception handler

A second except Exception as exc: immediately follows an identical catch-all handler, so it is unreachable dead code. The first handler already catches every exception. This looks like an accidental duplication from a restack and can trip linters.

Suggested change
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
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +330 to +361
hostname = parsed.hostname
if not hostname:
raise ValueError("vision client URL is missing a hostname")
if parsed.username or parsed.password:
raise ValueError("vision client URL must not contain user credentials")
try:
parsed.port
except ValueError as exc:
raise ValueError("vision client URL has an invalid port") from exc
normalized_hostname = hostname.rstrip(".").casefold()
if (
normalized_hostname in _BLOCKED_VISION_HOSTNAMES
or normalized_hostname.endswith(".localhost")
):
raise ValueError(
"vision client URL points to a private, loopback, link-local, or metadata destination"
)
try:
address = ipaddress.ip_address(hostname)
except ValueError:
address = None
if address is not None and (
address.is_private
or address.is_loopback
or address.is_link_local
or address.is_reserved
or address.is_multicast
or address.is_unspecified
):
raise ValueError(
"vision client URL points to a private, loopback, link-local, or metadata destination"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Vision channel silently disabled for IP-literal orchestrator URLs

The vision client constructor now rejects loopback, private, link-local, reserved, and metadata hosts even under allow_insecure_http=True. Since orchestrator_vision_client catches ValueError and returns NullImageContentClient, an orchestrator configured at 127.0.0.1 or localhost silently loses vision while embedding and adjudication keep working through the same URL. Compose's orchestrator DNS name is unaffected.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +71 to +80
with traced(
"lineageweave.valkey.activity_xadd",
{"db.system": "redis", "db.operation.name": "xadd", "lineageweave.stream.kind": "activity"},
):
return await client.xadd(
_stream_key(post_id),
_activity_fields(event_type, actor_account_id, summary),
maxlen=1000,
approximate=True,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Synchronous span held across await points

traced starts a synchronous current span and wraps awaited Valkey calls (e.g. publish_activity_event awaits client.xadd inside the with). Holding a current span across an await can attach concurrently scheduled tasks to the wrong parent span. Harmless for the current single-producer/reader paths; trace parenting could mislead if concurrency grows.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant