Update/epcis v2#34
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@zsculac New commits look good, maybe just add to readme somewhere that publisher_url needs to be setup? |
Adds OpenAI Codex-powered code review that runs on every PR and posts inline comments like a human reviewer. Uses structured output schema for reliable parsing and GitHub Review API for native inline comments. - .codex/review-prompt.md: Two-pass review (blockers then maintainability) with comment gate, deduplication, and uncertainty guard - .codex/review-schema.json: Strict output schema (additionalProperties: false) - .github/workflows/codex-review.yml: SHA-pinned actions, fork PR guard, paginated file listing, null-patch handling, overflow comments in summary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If Codex returns empty or non-JSON output, the workflow now posts a warning comment with a link to the logs instead of silently failing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tput Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds substantial EPCIS query/asset functionality and improves responses, but there are two blockers: the new GROUP_CONCAT query drops epcList when filtering by EPC, and the new user-facing routes/params have no tests. There are also schema/validation mismatches (OpenAPI response missing requestId, handler-side validation, and missing MCP input constraints) that should be cleaned up to stay consistent with project conventions. Overall the direction is good, but these correctness and test gaps need to be addressed first.
| return `${PREFIXES} | ||
| SELECT ?ual ?eventType ?eventTime ?epc ?bizStep ?disposition ?readPoint ?bizLocation | ||
| SELECT ?ual ?eventType ?eventTime ?bizStep ?bizLocation ?disposition ?readPoint ?action ?parentID | ||
| (GROUP_CONCAT(DISTINCT ?epc; SEPARATOR=", ") AS ?epcList) |
There was a problem hiding this comment.
🔴 Bug: ?epc is only bound when no epc filter is provided, so when callers filter by epc the GROUP_CONCAT(DISTINCT ?epc …) yields an empty epcList. Bind ?epc regardless of filters (e.g., always add OPTIONAL { ?event epcis:epcList ?epc . } and keep the filter separate) or change the SELECT to use a variable that’s always bound.
EPCIS V2 Reworked
There was a problem hiding this comment.
Two blockers: the publisher endpoint now depends on a frontend env var and the /epcis/events route introduces a breaking requirement for at least one filter. The refactor otherwise improves structure (shared validation/services and richer tests), but there are a couple of spec/test pattern drifts that should be cleaned up. Maintainability overall improves in the EPCIS plugin, though the OpenAPI schema mismatch and test import patterns slightly erode consistency.
Additional comments
These comments target lines outside the diff and could not be posted inline.
apps/agent/tests/unit/thinkingIndicatorVisibility.spec.ts:5— 🟡 Issue: This test imports fromsrc, which violates the repo rule to test againstdistartifacts. Consider exportingthinkingIndicatorfrom the built output or relocating the test to align with the guideline.
There was a problem hiding this comment.
This PR is largely clean and adds helpful validation/refactoring plus thorough EPCIS tests, but there are a few maintainability/compatibility issues to address. The biggest risks are the new hard dependency on EXPO_PUBLIC_MCP_URL for publisher calls and a test importing from src/ instead of dist/, which can mask build-time regressions. Maintainability in the EPCIS plugin is improved overall (clearer helpers and parity tests), while the chat UI changes are neutral aside from a small typing/test-structure cleanup.
Additional comments
These comments target lines outside the diff and could not be posted inline.
apps/agent/src/components/Chat/Messages.tsx:65— 🟡 Issue:React.MutableRefObjectis referenced without importingReact(or the type), which will failcheck-typesunder the new JSX transform. Importtype { MutableRefObject }fromreact(or useRefObject) and cast to that instead ofReact.*.apps/agent/tests/unit/thinkingIndicatorVisibility.spec.ts:20— 🟡 Issue: This new unit test doesn’t follow the requiredCore FunctionalityandError Handlingdescribe blocks used elsewhere inapps/agent/tests/unit. Please split these cases into those sections to match the testing convention.
There was a problem hiding this comment.
🔴 There is a correctness issue in the new SPARQL query: the grouping can collapse multiple events into a single row, which will undercount and merge results. Everything else looks directionally solid, with good validation and expanded tests/docs. Maintainability is generally improved via shared helpers and clearer error handling, though the stricter publisher endpoint config handling could be disruptive if existing deployments relied on the old fallback.
|
|
||
| // Assemble the query with GROUP_CONCAT for array fields | ||
| return `${PREFIXES} | ||
| SELECT ?ual ?eventType ?eventTime ?bizStep ?bizLocation ?disposition ?readPoint ?action ?parentID |
There was a problem hiding this comment.
🔴 Bug: This query groups without ?event, so distinct events with the same type/time/location/etc. will be merged by GROUP_CONCAT, losing rows and undercounting results. Include ?event in the SELECT/GROUP BY (and strip it from API responses if you don’t want to expose it) to preserve one row per event.
3 participants
No description provided.