Skip to content

fix(explorer): /api/decisions returns 422 — coerce decision timestamp to str - #937

Open
logan-jl-cc wants to merge 2 commits into
semantica-agi:mainfrom
logan-jl-cc:fix/explorer-decision-timestamp
Open

fix(explorer): /api/decisions returns 422 — coerce decision timestamp to str#937
logan-jl-cc wants to merge 2 commits into
semantica-agi:mainfrom
logan-jl-cc:fix/explorer-decision-timestamp

Conversation

@logan-jl-cc

Copy link
Copy Markdown

Problem

GET /api/decisions returns HTTP 422 (Unprocessable Entity) for any graph that contains real decisions, which makes the Decisions workspace in the Knowledge Explorer fail to load entirely — no decision can ever be listed.

Root cause

ContextGraph.record_decision() stores the decision timestamp as a POSIX float (e.g. 1786513069.69):

# semantica/context/context_graph.py — record_decision
metadata["timestamp"] = datetime.now().timestamp()   # float

But the explorer response model types it as Optional[str]:

# semantica/explorer/schemas.py
class DecisionResponse(BaseModel):
    ...
    timestamp: Optional[str] = None

_node_to_decision() in semantica/explorer/routes/decisions.py passes the raw float through:

timestamp=properties.get("timestamp"),   # float, not str

Pydantic (strict mode) rejects the float → str coercion and raises a validation error, so the endpoint returns 422:

ValueError: 1 validation error for DecisionResponse
timestamp
  Input should be a valid string [type=string_type, input_value=1786513069.69, input_type=float]

This affects every endpoint that builds a DecisionResponse (/api/decisions, /api/decisions/{id}, /api/decisions/{id}/precedents).

Fix

Coerce the timestamp to str at the boundary (preserving None), so the value matches the declared schema regardless of how the source store serializes it:

timestamp=None if properties.get("timestamp") is None else str(properties.get("timestamp")),

Minimal, one-line, no schema change.

Verification

Before — GET /api/decisions on a graph with 3 recorded decisions:

HTTP/1.1 422 Unprocessable Entity
{"detail":"Invalid input"}

After — same request:

HTTP/1.1 200 OK
[ {"decision_id":"…","category":"loan_underwriting","outcome":"approved","confidence":0.94,"timestamp":"1786513069.69", ...}, ... ]

The Decisions workspace then renders all 3 decisions, and clicking one opens its causal chain (Causal Chain · 7 steps) as expected.

Notes

  • Did not change DecisionResponse.timestamp typing (kept Optional[str]); the explorer frontend already consumes it as a string. Coercing at the adapter is the lowest-risk fix.
  • An alternative would be to widen the field to Optional[Union[str, float]], but that pushes serialization ambiguity onto every consumer; coercing at the single boundary is cleaner.

Checklist

  • Fix is minimal (1 line) and scoped to the explorer route adapter
  • No schema or API-shape change
  • Verified manually against a real ContextGraph with recorded decisions
  • Unit test for _node_to_decision with a float timestamp (happy to add if maintainers want)

…i/decisions

ContextGraph stores decision timestamps as POSIX floats (e.g. 1786513069.69),
but DecisionResponse.timestamp is typed Optional[str]. Pydantic strict
validation rejects the float and the whole /api/decisions endpoint returns
HTTP 422 "Invalid input", which breaks the Decisions workspace in the
Knowledge Explorer entirely (no decision can be listed).

Coerce the value to str (preserving None) in _node_to_decision so the
response validates. Verified: /api/decisions now returns 200 and the 3
sample decisions render in the Decisions workspace.
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix /api/decisions 422 by coercing decision timestamp to string

🐞 Bug fix 🕐 Less than 10 minutes

Grey Divider

AI Description

• Prevent 422 responses from Decisions API by matching response schema types.
• Coerce stored float decision timestamps to strings at the route adapter boundary.
Diagram

graph TD
  A["GET /api/decisions"] --> B["_node_to_decision()"] --> C["DecisionResponse (Pydantic)"] --> D["JSON 200 response"]
  B --> E["timestamp: float|None"]
  E --> F["timestamp: str|None"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Widen schema type (Union[str, float])
  • ➕ Avoids explicit coercion in the adapter
  • ➕ Accepts multiple timestamp serializations
  • ➖ Pushes ambiguity onto all API consumers
  • ➖ Can complicate client handling and documentation of the API contract
2. Normalize at write time (store timestamps as strings)
  • ➕ Single canonical representation in storage
  • ➕ Eliminates repeated coercion across adapters
  • ➖ Larger behavioral change; may affect existing graphs/data and any readers expecting numeric timestamps
  • ➖ Requires auditing all writers/serializers, not just explorer responses

Recommendation: Keep the current approach (coerce float→str in the explorer adapter). It is the smallest, lowest-risk fix that restores endpoint stability while preserving the existing API contract (timestamp as Optional[str]) and avoiding ambiguity for downstream consumers.

Files changed (1) +1 / -1

Bug fix (1) +1 / -1
decisions.pyCoerce decision timestamp to string in DecisionResponse adapter +1/-1

Coerce decision timestamp to string in DecisionResponse adapter

• Updates _node_to_decision() to convert a float timestamp property into a string (preserving None). This prevents strict Pydantic validation errors that previously caused 422 responses for decisions endpoints.

semantica/explorer/routes/decisions.py

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Inconsistent timestamp in metadata 🐞 Bug ≡ Correctness
Description
_node_to_decision() now stringifies the top-level timestamp but still returns the raw node
properties as metadata, so responses can contain timestamp as str while metadata['timestamp']
remains a float. This creates inconsistent API output and can break consumers that read/compare the
timestamp from metadata instead of the top-level field.
Code

semantica/explorer/routes/decisions.py[R26-27]

+        timestamp=None if properties.get("timestamp") is None else str(properties.get("timestamp")),
        metadata=properties,
Evidence
The diff changes only the top-level timestamp field to str(...) while leaving
metadata=properties untouched, so if properties['timestamp'] is a float, the response will
contain both a stringified timestamp and the original float in metadata. The graph code shows
decisions are recorded with a float timestamp, making this mismatch likely in real data.

semantica/explorer/routes/decisions.py[17-28]
semantica/explorer/schemas.py[137-146]
semantica/context/context_graph.py[2541-2565]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_node_to_decision()` coerces `DecisionResponse.timestamp` to `str`, but `metadata` is set to the original `properties` dict. When the stored value is a float (as produced by `ContextGraph.record_decision()`), the response contains two different timestamp types (`timestamp: str` vs `metadata.timestamp: float`).

### Issue Context
- This PR intentionally coerces the boundary to satisfy `DecisionResponse.timestamp: Optional[str]`.
- The endpoint also returns `metadata=properties`, which includes the same `timestamp` key.

### Fix Focus Areas
- semantica/explorer/routes/decisions.py[17-28]

### Suggested fix
1. Read the timestamp once into a local variable.
2. Build a new `metadata` dict (copy) and normalize or remove the `timestamp` key so it matches the top-level field.

Example:
```py
properties = node.get("properties", {})
ts_value = properties.get("timestamp")
ts = None if ts_value is None else str(ts_value)
metadata = dict(properties)
metadata["timestamp"] = ts  # or: metadata.pop("timestamp", None)

return DecisionResponse(
   ...,
   timestamp=ts,
   metadata=metadata,
)
```
Optionally add a small unit test for `_node_to_decision()` with `properties['timestamp']` as `float` to lock in the behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +26 to 27
timestamp=None if properties.get("timestamp") is None else str(properties.get("timestamp")),
metadata=properties,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Inconsistent timestamp in metadata 🐞 Bug ≡ Correctness

_node_to_decision() now stringifies the top-level timestamp but still returns the raw node
properties as metadata, so responses can contain timestamp as str while metadata['timestamp']
remains a float. This creates inconsistent API output and can break consumers that read/compare the
timestamp from metadata instead of the top-level field.
Agent Prompt
### Issue description
`_node_to_decision()` coerces `DecisionResponse.timestamp` to `str`, but `metadata` is set to the original `properties` dict. When the stored value is a float (as produced by `ContextGraph.record_decision()`), the response contains two different timestamp types (`timestamp: str` vs `metadata.timestamp: float`).

### Issue Context
- This PR intentionally coerces the boundary to satisfy `DecisionResponse.timestamp: Optional[str]`.
- The endpoint also returns `metadata=properties`, which includes the same `timestamp` key.

### Fix Focus Areas
- semantica/explorer/routes/decisions.py[17-28]

### Suggested fix
1. Read the timestamp once into a local variable.
2. Build a new `metadata` dict (copy) and normalize or remove the `timestamp` key so it matches the top-level field.

Example:
```py
properties = node.get("properties", {})
ts_value = properties.get("timestamp")
ts = None if ts_value is None else str(ts_value)
metadata = dict(properties)
metadata["timestamp"] = ts  # or: metadata.pop("timestamp", None)

return DecisionResponse(
    ...,
    timestamp=ts,
    metadata=metadata,
)
```
Optionally add a small unit test for `_node_to_decision()` with `properties['timestamp']` as `float` to lock in the behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@Sameer6305

Copy link
Copy Markdown
Collaborator

@logan-jl-cc can handle the qodo reviews before we review it.

Regression tests for the 422 fix in _node_to_decision. Covers the cases
that produced HTTP 422 (float / int timestamps from ContextGraph) and
the ones that must keep working (None, already-string, missing key).

Verified the suite catches the regression: with the fix reverted, the
float / int / nan / inf cases fail with the same ValidationError that
caused the 422; with the fix applied all 6 pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants