fix: keep visual whitespace out of authoritative structure - #319
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 9f7574f. Check the declared-vs-source indentation boundary, table/list regressions, and current required Checks. Report actionable findings only; do not approve, bypass, or use stale parent evidence. |
|
Review exact current HEAD 9f7574f. The whitespace-to-semantic-structure boundary is a dependency for adjacent table and visual-region work; required Full test suite and Frontend lint, test, build checks are green. Please publish a formal independent review for this exact SHA. |
… into codex/normalize-source-indent-semantics # Conflicts: # docs/product-technical-gap-baseline.md
|
Review exact current HEAD |
… into codex/normalize-source-indent-semantics
|
@OpenCode review exact current head |
|
Reviewed against exact current head
The PR remains blocked pending independent formal review and terminal Checks; no bypass used. |
|
Correction: the exact current head is |
… into pr319-strix-observability
|
The branch has moved to exact current HEAD |
|
@opencode-agent Review exact current head |
… into codex/review-pr319 # Conflicts: # docs/product-technical-gap-baseline.md
![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.
* 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
a4c23c6
into
codex/post-structure-case-fixes
There was a problem hiding this comment.
📝 Info: Narrowed exception set may let rare transport errors escape isolation
The batch failure handling changed from a broad except Exception to except (OSError, RuntimeError, ValueError) in both the structure loop (post_content_persistence.py) and the embedding loop (post_content_persistence.py). This is intentional per ADR 0103/the changelog (AssertionError and other defects should propagate to the durable worker). I verified the common expected transport failures are still isolated: HttpClientError subclasses RuntimeError (http_client.py), TimeoutError/ConnectionError/ssl.SSLError are OSError subclasses, and response validation raises ValueError. The residual edge is that a bare http.client.HTTPException subclass (e.g. IncompleteRead, BadStatusLine) is neither OSError/RuntimeError/ValueError, so a genuinely transient transport hiccup of that kind would now propagate and fail the whole post job rather than leaving the batch retryable. This is a low-probability edge and aligns with the documented decision, so not flagged as a bug — noting for awareness.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
📝 Info: Adjudicated and explicit indent levels use independent numbering
With source-only whitespace now routed to adjudication instead of being treated as explicit, a document can end up with both declared-indent chunks (whose indent_level comes from explicit_levels in post_content_persistence.py) and orchestrator-adjudicated chunks (whose indent_level comes from client.infer). These two numbering schemes are independent and may not be consistently ordered relative to each other, so a mixed document could persist inconsistent nesting levels across the explicit and adjudicated units. This is not introduced as a new defect by this PR (the adjudication path already numbered independently), but the change makes the mixed-source scenario more common. Worth confirming the buyer view tolerates the two schemes coexisting.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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
chat_about_post now has two identical except Exception as exc: handlers back to back. The first already catches everything, so the second (main.py) is unreachable dead code, an apparent merge artifact.
| 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.
| _INTERNAL_IMAGE_INSTRUCTION = re.compile( | ||
| r"^(?:" | ||
| r"this post is an image(?:\s*[.!?]|.*(?:ask\s+questions|read\s+its\s+text).*)" | ||
| r"|이 글의 이미지입니다(?:\s*[.!?]|.*(?:keyman\s*(?:을|를)\s*추출|질문해.*텍스트를\s*읽으세요|이미지\s*안의\s*텍스트를\s*읽으세요).*)" | ||
| r"|(?:this image|이 이미지는).*(?:keyman\s*(?:을|를)\s*추출|ask\s+questions|read\s+its\s+text|질문해.*(?:읽으세요|추출하세요)|텍스트를\s*(?:읽으세요|추출하세요)).*" | ||
| r")$", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
🔍 Unpunctuated internal instruction can leak
_INTERNAL_IMAGE_INSTRUCTION matches this post is an image only when followed by punctuation or an instruction phrase. A caption that is exactly This post is an image with no trailing text passes through unsuppressed. Whether such a bare caption exists in persisted data determines if this matters.
Was this helpful? React with 👍 or 👎 to provide feedback.
| with traced( | ||
| "lineageweave.http.post_json", | ||
| { | ||
| "http.request.method": "POST", | ||
| "server.address": hostname, | ||
| "url.path": parsed.path or "/", | ||
| "service.peer.name": "contextual-orchestrator", | ||
| }, | ||
| ) as span: | ||
| inject_trace_context(request_headers) | ||
| status, raw = _request( | ||
| "POST", | ||
| url, | ||
| body=json.dumps(request_payload).encode("utf-8"), | ||
| headers=request_headers, | ||
| timeout=timeout, | ||
| ) | ||
| if span is not None: | ||
| span.set_attribute("http.response.status_code", status) | ||
| if status >= 400: | ||
| raise HttpClientError(f"HTTP {status} from {hostname}") | ||
| return _decode_json_object(raw, hostname) |
There was a problem hiding this comment.
📝 Info: HTTP error status not recorded on span
In post_json, the status >= 400 raise and the JSON decode run after the traced span closes (http_client.py). The span records only http.response.status_code; 4xx/5xx and decode failures never set span error status. Correctness is unaffected.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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" | ||
| ) |
There was a problem hiding this comment.
📝 Info: Localhost vision endpoint silently disabled
The vision client now rejects loopback/localhost/metadata hosts even with allow_insecure_http=True, and orchestrator_vision_client swallows the ValueError into NullImageContentClient. A dev pointing the orchestrator at 127.0.0.1/localhost loses vision while other channels keep working. The Compose default uses host orchestrator, and ADR 0121 states this is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Buyer-visible gap
Rich-text exports that use
or leading spaces for visual alignment were persisted as progressively deeper explicit nesting. Mixed editor whitespace could therefore distort list hierarchy in the buyer view.Change
explicitstructure only from declared indentation; source-only whitespace uses contextual-orchestrator adjudication and otherwise remains unresolved at level zero.Exact stack
codex/post-structure-case-fixes).1faed2f680fa6d7bbe946a1a884522757d1639d6.e3f00eaae9255f5f56eaa5d93b6fa2ea6ea3e8c5.Verification boundary
733 passed, 16 skipped, 4 warnings.136tests, production build, Storybook,actionlint, andgit diff --checkpassed on the predecessor product-equivalent tree.No real records, identifiers, or provider credentials are included. Merge only after #317 lands, exact-head Checks pass, and an independent exact-head approval exists.