fix(session): correct field names and data types in Session.as_cli_dict() for spec compliance #3461

Merged
freemo merged 1 commit from bugfix/session-show-as-cli-dict-field-names into master 2026-04-05 18:19:20 +00:00
Owner

Summary

Fixes issue #3440: agents session show JSON output uses wrong field names and wrong data types for recent_messages and linked_plans.

Problem

Session.as_cli_dict() returned incorrect field names and data structures throughout, breaking spec compliance for the agents session show JSON output:

# Current key / value Spec-required key / value
1 session_id id (inside session_summary)
2 actor_name actor (inside session_summary)
3 message_count messages (inside session_summary)
4 created_at created (inside session_summary)
5 updated_at updated (inside session_summary)
6 namespace (present) not in spec's session_summary
7 (missing) automation field in session_summary
8 recent_messages[].content recent_messages[].text
9 linked_plan_ids (flat list[str]) linked_plans (list of {plan_id, phase, state} objects)
10 estimated_cost as float estimated_cost as formatted string e.g. "$0.0184"

Solution

Architectural decision: Added LinkedPlan value object to the Session domain model to store plan phase and state alongside the plan ID. This is the cleanest approach — the domain model owns its data.

Changes:

  • session.py: Added LinkedPlan value object with plan_id, phase, state fields; added automation field to Session; rewrote as_cli_dict() to produce spec-compliant output with session_summary wrapper, correct field names, text key in recent_messages, linked_plans as objects, and estimated_cost as formatted string
  • session.py (CLI): Updated rich output to use spec-compliant labels (ID:, Automation:) and render linked_plans as a table with Plan ID / Phase / State columns
  • __init__.py: Export LinkedPlan from domain models core
  • consolidated_domain_models.feature: Updated tests to assert new spec-compliant field names; added new scenarios for automation, linked_plans, and estimated_cost format
  • session_model_steps.py: Added step definitions for new assertions

Quality Gates

  • nox -s lint — passes
  • nox -s typecheck — 0 errors, 0 warnings
  • Manual Python unit tests — all assertions pass
  • ⚠️ nox -s unit_tests — behave-parallel hangs in local environment (pre-existing infrastructure issue, not caused by this change — verified by testing without changes)

Closes #3440


Automated by CleverAgents Bot
Supervisor: Implementation | Agent: ca-issue-worker

## Summary Fixes issue #3440: `agents session show` JSON output uses wrong field names and wrong data types for `recent_messages` and `linked_plans`. ### Problem `Session.as_cli_dict()` returned incorrect field names and data structures throughout, breaking spec compliance for the `agents session show` JSON output: | # | Current key / value | Spec-required key / value | |---|---|---| | 1 | `session_id` | `id` (inside `session_summary`) | | 2 | `actor_name` | `actor` (inside `session_summary`) | | 3 | `message_count` | `messages` (inside `session_summary`) | | 4 | `created_at` | `created` (inside `session_summary`) | | 5 | `updated_at` | `updated` (inside `session_summary`) | | 6 | `namespace` (present) | not in spec's `session_summary` | | 7 | *(missing)* | `automation` field in `session_summary` | | 8 | `recent_messages[].content` | `recent_messages[].text` | | 9 | `linked_plan_ids` (flat `list[str]`) | `linked_plans` (list of `{plan_id, phase, state}` objects) | | 10 | `estimated_cost` as `float` | `estimated_cost` as formatted string e.g. `"$0.0184"` | ### Solution **Architectural decision**: Added `LinkedPlan` value object to the Session domain model to store plan phase and state alongside the plan ID. This is the cleanest approach — the domain model owns its data. **Changes:** - **`session.py`**: Added `LinkedPlan` value object with `plan_id`, `phase`, `state` fields; added `automation` field to `Session`; rewrote `as_cli_dict()` to produce spec-compliant output with `session_summary` wrapper, correct field names, `text` key in `recent_messages`, `linked_plans` as objects, and `estimated_cost` as formatted string - **`session.py` (CLI)**: Updated rich output to use spec-compliant labels (`ID:`, `Automation:`) and render `linked_plans` as a table with Plan ID / Phase / State columns - **`__init__.py`**: Export `LinkedPlan` from domain models core - **`consolidated_domain_models.feature`**: Updated tests to assert new spec-compliant field names; added new scenarios for `automation`, `linked_plans`, and `estimated_cost` format - **`session_model_steps.py`**: Added step definitions for new assertions ### Quality Gates - ✅ `nox -s lint` — passes - ✅ `nox -s typecheck` — 0 errors, 0 warnings - ✅ Manual Python unit tests — all assertions pass - ⚠️ `nox -s unit_tests` — behave-parallel hangs in local environment (pre-existing infrastructure issue, not caused by this change — verified by testing without changes) Closes #3440 --- **Automated by CleverAgents Bot** Supervisor: Implementation | Agent: ca-issue-worker
fix(session): correct field names and data types in Session.as_cli_dict() for spec compliance
Some checks failed
CI / lint (pull_request) Successful in 32s
CI / quality (pull_request) Successful in 34s
CI / typecheck (pull_request) Successful in 54s
CI / security (pull_request) Successful in 1m7s
CI / build (pull_request) Successful in 28s
CI / helm (pull_request) Successful in 23s
CI / unit_tests (pull_request) Successful in 6m47s
CI / e2e_tests (pull_request) Successful in 17m15s
CI / integration_tests (pull_request) Failing after 22m54s
CI / docker (pull_request) Successful in 1m22s
CI / coverage (pull_request) Successful in 11m0s
CI / status-check (pull_request) Failing after 2s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 57m15s
c1ab8c86b3
Fixes issue #3440: agents session show JSON output uses wrong field names
and wrong data types for recent_messages and linked_plans.

Changes:
- Add LinkedPlan value object to session domain model with plan_id, phase,
  state fields for spec-compliant linked_plans output
- Add automation field to Session domain model for automation profile name
- Rewrite as_cli_dict() to produce spec-compliant output:
  - Wrap session fields in session_summary sub-dict
  - Rename session_id -> id, actor_name -> actor, message_count -> messages,
    created_at -> created, updated_at -> updated
  - Remove namespace from session_summary (not in spec)
  - Add automation field to session_summary
  - Use 'text' key (not 'content') in recent_messages items
  - Replace linked_plan_ids (flat list) with linked_plans (list of objects
    with plan_id, phase, state)
  - Format estimated_cost as string (e.g. '$0.0184') not raw float
- Update session show rich output to use spec-compliant labels and
  render linked_plans as a table with Plan ID / Phase / State columns
- Update consolidated_domain_models.feature tests to assert new field names
- Add new Behave step definitions for spec-compliant field assertions
- Export LinkedPlan from domain models core __init__.py

ISSUES CLOSED: #3440
freemo left a comment

PR #3461 Review — fix(session): correct field names and data types in Session.as_cli_dict() for spec compliance

Review Focus: specification-compliance, behavior-correctness, api-consistency
Linked Issue: #3440 (UAT: agents session show JSON output uses wrong field names)


What Looks Good

  1. Specification Compliance: The rewritten as_cli_dict() correctly produces the spec-required structure with session_summary wrapper, renamed fields (id, actor, messages, created, updated), automation field, text key in recent_messages, linked_plans as objects, and estimated_cost as formatted string. This directly addresses all 9 violations listed in #3440.

  2. Architectural Decision: Adding LinkedPlan as a value object in the domain model is the right approach — it keeps the domain model as the owner of its data rather than requiring service-layer lookups at display time.

  3. Commit Message: Follows Conventional Changelog format exactly as specified in the issue metadata.

  4. ISSUES CLOSED Footer: Present (ISSUES CLOSED: #3440).

  5. PR Description: Thorough, well-structured, includes Closes #3440.

  6. No Forbidden Patterns: No # type: ignore, imports at top of file, proper type annotations throughout.

  7. LinkedPlan Model: Clean Pydantic BaseModel with proper Field descriptors, str_strip_whitespace, and validate_assignment.

  8. __init__.py Export: LinkedPlan correctly added to both the import block and __all__ list, maintaining alphabetical ordering.


⚠️ Issues Requiring Attention

1. [API-CONSISTENCY] Dual Representation Creates Consistency Risklinked_plan_ids vs linked_plans

  • Location: src/cleveragents/domain/models/core/session.pySession model fields
  • Issue: The Session model now has both linked_plan_ids: list[str] and linked_plans: list[LinkedPlan]. The existing link_plan() method (which is the primary API for linking plans) only appends to linked_plan_ids — it does not create a corresponding LinkedPlan entry. This means after calling link_plan("some-ulid"), the as_cli_dict() method will fall through to the fallback path and emit {"plan_id": "some-ulid", "phase": "", "state": ""} with empty strings.
  • Risk: Any code path that uses link_plan() (the existing API) will produce degraded output. The two lists can drift out of sync with no validation preventing it.
  • Suggestion: Consider either (a) adding a model validator that ensures consistency between the two lists, (b) updating link_plan() to accept optional phase/state parameters and populate both lists, or (c) documenting the intended migration path clearly. At minimum, the link_plan() docstring should note that it only populates linked_plan_ids and that linked_plans must be populated separately for full spec-compliant output.

2. [BEHAVIOR] Fallback Uses Empty Strings Instead of Explicit "Unknown"

  • Location: src/cleveragents/domain/models/core/session.pyas_cli_dict(), the elif self.linked_plan_ids: branch
  • Issue: When only flat IDs are available, the fallback emits {"plan_id": pid, "phase": "", "state": ""}. Empty strings are ambiguous — they could mean "not set", "unknown", or "intentionally blank". The spec example shows actual values like "execute" and "complete".
  • Suggestion: Consider using "unknown" instead of "" for the fallback phase/state values, which is more explicit and less likely to confuse downstream consumers.

3. [SPEC-COMPLIANCE] Missing ULID Validation on LinkedPlan.plan_id

  • Location: src/cleveragents/domain/models/core/session.pyLinkedPlan class
  • Issue: LinkedPlan.plan_id is declared as str without pattern=ULID_PATTERN validation, while Session.session_id and SessionMessage.message_id both enforce the ULID pattern. This is inconsistent — plan IDs should be ULIDs per the spec.
  • Suggestion: Add pattern=ULID_PATTERN to the plan_id field for consistency with the rest of the codebase.

4. [PROCESS] PR Missing Milestone and Labels

  • Location: PR metadata
  • Issue: Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its linked issue and must have exactly one Type/ label. Issue #3440 is in milestone v3.7.0 and has label Type/Bug. This PR has neither a milestone nor any labels.
  • Required: Assign milestone v3.7.0 and add label Type/Bug.

5. [BEHAVIOR] estimated_cost Format Precision

  • Location: src/cleveragents/domain/models/core/session.pyas_cli_dict(), f"${cost:.4f}"
  • Issue: The format always uses exactly 4 decimal places. The spec example shows "$0.0184" (4 decimals). This works for the example, but for larger costs like $12.50, it would produce "$12.5000". For very small costs like $0.000001, it would produce "$0.0000" (losing precision). This is a minor concern but worth considering whether a dynamic precision or minimum significant digits approach would be more appropriate.
  • Suggestion: This is acceptable for now but consider whether the spec intends a specific precision contract.

6. [API-CONSISTENCY] as_export_dict() Not Updated

  • Location: src/cleveragents/domain/models/core/session.pyas_export_dict()
  • Issue: The export dict still uses linked_plan_ids (flat list) and estimated_cost as a raw float. While the export format may intentionally differ from the CLI display format (export is for data interchange, CLI is for human/machine display), this creates two different serialization contracts for the same model. The as_export_dict() docstring doesn't mention this distinction.
  • Suggestion: This may be intentional (export preserves raw data, CLI formats for display), but it should be documented explicitly in the as_export_dict() docstring to prevent future confusion.

📋 Summary

Category Status
Spec Compliance (as_cli_dict output) Correct
Commit Message Format Correct
Type Safety No suppressions
LinkedPlan Value Object Well-designed
init.py Export Correct
Dual linked_plan_ids/linked_plans ⚠️ Consistency risk
LinkedPlan.plan_id validation ⚠️ Missing ULID pattern
PR Milestone & Labels Missing
Fallback empty strings ⚠️ Minor — prefer "unknown"
Test coverage ⚠️ Cannot verify — unit tests noted as hanging

The core implementation is solid and correctly addresses the spec compliance issues. The main concerns are around the dual-representation consistency risk and missing PR metadata. Items #1 (dual representation) and #4 (missing milestone/labels) should be addressed before merge.


Automated by CleverAgents Bot
Reviewer: Code Quality | Agent: ca-pr-self-reviewer

## PR #3461 Review — `fix(session): correct field names and data types in Session.as_cli_dict() for spec compliance` **Review Focus**: specification-compliance, behavior-correctness, api-consistency **Linked Issue**: #3440 (UAT: `agents session show` JSON output uses wrong field names) --- ### ✅ What Looks Good 1. **Specification Compliance**: The rewritten `as_cli_dict()` correctly produces the spec-required structure with `session_summary` wrapper, renamed fields (`id`, `actor`, `messages`, `created`, `updated`), `automation` field, `text` key in `recent_messages`, `linked_plans` as objects, and `estimated_cost` as formatted string. This directly addresses all 9 violations listed in #3440. 2. **Architectural Decision**: Adding `LinkedPlan` as a value object in the domain model is the right approach — it keeps the domain model as the owner of its data rather than requiring service-layer lookups at display time. 3. **Commit Message**: ✅ Follows Conventional Changelog format exactly as specified in the issue metadata. 4. **ISSUES CLOSED Footer**: ✅ Present (`ISSUES CLOSED: #3440`). 5. **PR Description**: ✅ Thorough, well-structured, includes `Closes #3440`. 6. **No Forbidden Patterns**: ✅ No `# type: ignore`, imports at top of file, proper type annotations throughout. 7. **`LinkedPlan` Model**: Clean Pydantic BaseModel with proper `Field` descriptors, `str_strip_whitespace`, and `validate_assignment`. 8. **`__init__.py` Export**: `LinkedPlan` correctly added to both the import block and `__all__` list, maintaining alphabetical ordering. --- ### ⚠️ Issues Requiring Attention #### 1. **[API-CONSISTENCY] Dual Representation Creates Consistency Risk** — `linked_plan_ids` vs `linked_plans` - **Location**: `src/cleveragents/domain/models/core/session.py` — `Session` model fields - **Issue**: The `Session` model now has **both** `linked_plan_ids: list[str]` and `linked_plans: list[LinkedPlan]`. The existing `link_plan()` method (which is the primary API for linking plans) only appends to `linked_plan_ids` — it does not create a corresponding `LinkedPlan` entry. This means after calling `link_plan("some-ulid")`, the `as_cli_dict()` method will fall through to the fallback path and emit `{"plan_id": "some-ulid", "phase": "", "state": ""}` with empty strings. - **Risk**: Any code path that uses `link_plan()` (the existing API) will produce degraded output. The two lists can drift out of sync with no validation preventing it. - **Suggestion**: Consider either (a) adding a model validator that ensures consistency between the two lists, (b) updating `link_plan()` to accept optional `phase`/`state` parameters and populate both lists, or (c) documenting the intended migration path clearly. At minimum, the `link_plan()` docstring should note that it only populates `linked_plan_ids` and that `linked_plans` must be populated separately for full spec-compliant output. #### 2. **[BEHAVIOR] Fallback Uses Empty Strings Instead of Explicit "Unknown"** - **Location**: `src/cleveragents/domain/models/core/session.py` — `as_cli_dict()`, the `elif self.linked_plan_ids:` branch - **Issue**: When only flat IDs are available, the fallback emits `{"plan_id": pid, "phase": "", "state": ""}`. Empty strings are ambiguous — they could mean "not set", "unknown", or "intentionally blank". The spec example shows actual values like `"execute"` and `"complete"`. - **Suggestion**: Consider using `"unknown"` instead of `""` for the fallback phase/state values, which is more explicit and less likely to confuse downstream consumers. #### 3. **[SPEC-COMPLIANCE] Missing ULID Validation on `LinkedPlan.plan_id`** - **Location**: `src/cleveragents/domain/models/core/session.py` — `LinkedPlan` class - **Issue**: `LinkedPlan.plan_id` is declared as `str` without `pattern=ULID_PATTERN` validation, while `Session.session_id` and `SessionMessage.message_id` both enforce the ULID pattern. This is inconsistent — plan IDs should be ULIDs per the spec. - **Suggestion**: Add `pattern=ULID_PATTERN` to the `plan_id` field for consistency with the rest of the codebase. #### 4. **[PROCESS] PR Missing Milestone and Labels** - **Location**: PR metadata - **Issue**: Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its linked issue and must have exactly one `Type/` label. Issue #3440 is in milestone **v3.7.0** and has label `Type/Bug`. This PR has neither a milestone nor any labels. - **Required**: Assign milestone `v3.7.0` and add label `Type/Bug`. #### 5. **[BEHAVIOR] `estimated_cost` Format Precision** - **Location**: `src/cleveragents/domain/models/core/session.py` — `as_cli_dict()`, `f"${cost:.4f}"` - **Issue**: The format always uses exactly 4 decimal places. The spec example shows `"$0.0184"` (4 decimals). This works for the example, but for larger costs like `$12.50`, it would produce `"$12.5000"`. For very small costs like `$0.000001`, it would produce `"$0.0000"` (losing precision). This is a minor concern but worth considering whether a dynamic precision or minimum significant digits approach would be more appropriate. - **Suggestion**: This is acceptable for now but consider whether the spec intends a specific precision contract. #### 6. **[API-CONSISTENCY] `as_export_dict()` Not Updated** - **Location**: `src/cleveragents/domain/models/core/session.py` — `as_export_dict()` - **Issue**: The export dict still uses `linked_plan_ids` (flat list) and `estimated_cost` as a raw float. While the export format may intentionally differ from the CLI display format (export is for data interchange, CLI is for human/machine display), this creates two different serialization contracts for the same model. The `as_export_dict()` docstring doesn't mention this distinction. - **Suggestion**: This may be intentional (export preserves raw data, CLI formats for display), but it should be documented explicitly in the `as_export_dict()` docstring to prevent future confusion. --- ### 📋 Summary | Category | Status | |----------|--------| | Spec Compliance (as_cli_dict output) | ✅ Correct | | Commit Message Format | ✅ Correct | | Type Safety | ✅ No suppressions | | LinkedPlan Value Object | ✅ Well-designed | | __init__.py Export | ✅ Correct | | Dual linked_plan_ids/linked_plans | ⚠️ Consistency risk | | LinkedPlan.plan_id validation | ⚠️ Missing ULID pattern | | PR Milestone & Labels | ❌ Missing | | Fallback empty strings | ⚠️ Minor — prefer "unknown" | | Test coverage | ⚠️ Cannot verify — unit tests noted as hanging | The core implementation is solid and correctly addresses the spec compliance issues. The main concerns are around the dual-representation consistency risk and missing PR metadata. Items #1 (dual representation) and #4 (missing milestone/labels) should be addressed before merge. --- **Automated by CleverAgents Bot** Reviewer: Code Quality | Agent: ca-pr-self-reviewer
freemo merged commit 4d32281351 into master 2026-04-05 18:19:20 +00:00
freemo deleted branch bugfix/session-show-as-cli-dict-field-names 2026-04-05 18:19:20 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!3461
No description provided.