Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Keep the reported value separate from the event timestamp: deadline | ||
| // changes carry the actual deadline in value, not timestamp. | ||
| timestamp, | ||
| value: attrs.value ?? null, |
There was a problem hiding this comment.
Structured value objects can still lose fields in actual MCP responses. wrapTool passes the whole result through stripResponseSteering, which recursively removes _agent_steering and _response_contract, plus fields such as recommendations beneath any _metadata key. For example, {"_metadata":{"recommendations":["reported"]}} becomes {"_metadata":{}}. This affects both the timeline and compact snapshot.
Keep reported event values outside this cleanup and add a test through the registered tool; the new executor tests stop before this step. Greptile automatically discovered a related ticket stating that structured event values must be preserved, which informed this comment.
Source Used: Linear — MCP transport events drop deadline values and provenance
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/tools/get-container-transport-events.ts
Line: 220
Comment:
**Reported fields can disappear**
Structured `value` objects can still lose fields in actual MCP responses. `wrapTool` passes the whole result through `stripResponseSteering`, which recursively removes `_agent_steering` and `_response_contract`, plus fields such as `recommendations` beneath any `_metadata` key. For example, `{"_metadata":{"recommendations":["reported"]}}` becomes `{"_metadata":{}}`. This affects both the timeline and compact snapshot.
Keep reported event values outside this cleanup and add a test through the registered tool; the new executor tests stop before this step. Greptile automatically discovered a related ticket stating that structured event values must be preserved, which informed this comment.
**Source Used:** Linear — [MCP transport events drop deadline values and provenance](https://linear.app/terminal49/issue/DEV-13524/mcp-transport-events-drop-deadline-values-and-provenance)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Not reachable in practice: stripResponseSteering only drops _agent_steering, _response_contract and steering keys under _metadata, and the only value_jsonb writer in tnt-api stores eta_source_selection.
There was a problem hiding this comment.
You're right — given the current producer invariant, this finding is not actionable. stripResponseSteering would only remove the known steering keys (and steering fields under _metadata), while the only value_jsonb writer produces eta_source_selection, which is not one of those keys. Therefore the structured value is not reachable in the reported form I described, and no change or additional registered-tool test is needed for this PR. I'll withdraw Issue 1; the separate return-type finding remains independent.
There was a problem hiding this comment.
Additional verification: a synthetic test through each registered handler reproduces the transformation from value: {"_metadata":{"recommendations":["reported"]}} to value: {"_metadata":{}} in both the timeline and snapshot. My earlier production-reachability qualification still applies: the current backend JSON writer emits eta_source_selection, so this is not evidence of current production data loss. However, the PR explicitly promises preservation of arbitrary structured values. If we retain that contract, scope steering cleanup to MCP-owned metadata and preserve reported event values, with a registered-handler regression test. Otherwise, narrow the preservation claim. I would treat this as contract hardening, not a demonstrated production blocker.
There was a problem hiding this comment.
That verification changes the conclusion: the finding is valid as contract hardening. The PR explicitly promises that value is preserved as supplied, including arbitrary structured JSON, so a registered-handler test that transforms {"_metadata":{"recommendations":["reported"]}} into {"_metadata":{}} is a real contract violation, even if the current backend writer only emits eta_source_selection.
Please preserve reported event values outside the steering cleanup, or scope that cleanup to MCP-owned metadata, and add regression coverage for both the dedicated timeline and snapshot handlers. I’m reinstating this as a non-blocking P2. The separate ContainerStatus return-type issue remains valid independently.
| value: latestEvent.value ?? null, | ||
| data_source: latestEvent.data_source ?? null, | ||
| invalidated_at: latestEvent.invalidated_at ?? null, |
There was a problem hiding this comment.
formatEventsData now returns value, data_source, and invalidated_at, but ContainerStatus.events.latest_event still declares only event, timestamp, and location. Since executeGetContainer returns Promise<ContainerStatus>, TypeScript callers cannot access the new fields without a cast.
Add these fields to the declared return type, using unknown for arbitrary reported value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/tools/get-container.ts
Line: 388-390
Comment:
**New fields lack types**
`formatEventsData` now returns `value`, `data_source`, and `invalidated_at`, but `ContainerStatus.events.latest_event` still declares only `event`, `timestamp`, and `location`. Since `executeGetContainer` returns `Promise<ContainerStatus>`, TypeScript callers cannot access the new fields without a cast.
Add these fields to the declared return type, using `unknown` for arbitrary reported `value`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Worth the three lines, though note formatEventsData already returns any, so the interface is only documentation either way.
There was a problem hiding this comment.
Agreed. Since formatEventsData currently returns any, these fields are primarily documentation today rather than added compile-time safety. I still consider the type update worthwhile because ContainerStatus is the public contract for executeGetContainer and records the shape callers should expect; it will also provide type checking if the formatter is tightened or refactored later.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
There was a problem hiding this comment.
One clarification to my earlier reply: formatEventsData(): any hides the mismatch inside the implementation, but callers still receive Promise<ContainerStatus>. Accessing result.events?.latest_event?.value therefore fails type checking today. Please add value: unknown, data_source: string | null, and invalidated_at: string | null to ContainerStatus.events.latest_event, and give formatEventsData a concrete return type such as NonNullable<ContainerStatus['events']> so future mismatches are checked.
There was a problem hiding this comment.
You’re right—both changes are valid. The public return type should expose the fields, and the formatter should be checked against that shape instead of returning any:
| value: latestEvent.value ?? null, | |
| data_source: latestEvent.data_source ?? null, | |
| invalidated_at: latestEvent.invalidated_at ?? null, | |
| latest_event?: { | |
| event: string; | |
| timestamp: string; | |
| value: unknown; | |
| data_source: string | null; | |
| invalidated_at: string | null; | |
| location?: string; | |
| }; |
| value: latestEvent.value ?? null, | |
| data_source: latestEvent.data_source ?? null, | |
| invalidated_at: latestEvent.invalidated_at ?? null, | |
| function formatEventsData( | |
| events: any[], | |
| ): NonNullable<ContainerStatus['events']> { |
This preserves arbitrary structured values while documenting the nullable provenance fields and making future formatter mismatches type-checkable.
maurycy
left a comment
maurycy
left a comment
There was a problem hiding this comment.
Confirmed against tnt-api: LineDetentionLastFreeDayChangedTransportEventCreator writes the deadline into value_timestamp and the report time into timestamp, so reading value as the deadline is correct.
| timestamp, | ||
| value: attrs.value ?? null, | ||
| data_source: attrs.data_source ?? null, | ||
| invalidated_at: attrs.invalidated_at ?? null, |
There was a problem hiding this comment.
Both endpoints return with_invalidated, and extractKeyMilestones, categorizeEvents and the snapshot's latest_event still treat invalidated events as live, so a withdrawn event can become a milestone or the latest event despite the description's promise.
| '(vessel loaded, departed, arrived, discharged, rail movements, delivery). ' + | ||
| 'Use this for questions about journey history, "what happened", timeline analysis, rail tracking. ' + | ||
| 'For reported deadline changes, including container.detention_lfd_line.changed (empty-return LFD), ' + | ||
| 'read value as the deadline, not timestamp. value is preserved as reported and may be null or non-date JSON. ' + |
There was a problem hiding this comment.
This is agent steering that belongs in resources/query-guidance.ts, not in a tool description sent on every tools/list; one sentence (deadline is in value, timestamp is when it was reported) is enough here.
| data: { id: 'container-1', type: 'container' }, | ||
| included: [event], | ||
| }); | ||
| const result = await executeGetContainerTransportEvents( |
There was a problem hiding this comment.
These 17 regression cases pass, but they call the executors directly and stop before wrapTool builds the actual MCP response. Please add a focused test through the registered handler for both get_container_transport_events and get_container, asserting that the reported value and provenance survive into the final response. The existing executor matrix can stay. If arbitrary structured JSON preservation remains part of the contract, include a nested object with keys that collide with stripResponseSteering; that case currently fails after wrapping, although today's backend producers do not emit it.
This branch was successfully deployed
2 participants
Summary
Fixes DEV-13524: preserve public transport-event
value,data_source, andinvalidated_atinstead of discarding them during MCP formatting.For
container.detention_lfd_line.changed, the deadline is the API'svalue, nottimestamp. Pickup LFD remains separate and unchanged. This fixes information loss where an event is reported; it does not establish carrier coverage or promise an empty-return deadline for every container. No t49/backend changes, agent prompt changes, auth changes, or SQL-tool dependencies.Verification
npm run checkpasses (existing lint warnings remain).git diff --checkpasses.Need help on this PR? Tag
@codesmith-botwith what you need.The PR appears safe to merge, with non-blocking fixes recommended for structured values and the snapshot return type.
Fix with agent prompt
Summary
Preserves reported transport-event values, sources, and withdrawal dates in timeline and compact snapshot responses.
Reviews (1) · Last reviewed commit: "fix(mcp): preserve transport event value..." · Reviewed by Greptile