Skip to content

fix: preserve review operations when curate output is truncated - #770

Open
WhiskeyLabs wants to merge 1 commit into
campfirein:mainfrom
WhiskeyLabs:fix/preserve-review-operations
Open

fix: preserve review operations when curate output is truncated#770
WhiskeyLabs wants to merge 1 commit into
campfirein:mainfrom
WhiskeyLabs:fix/preserve-review-operations

Conversation

@WhiskeyLabs

Copy link
Copy Markdown

Summary

  • Problem: Successful curation can lose its structured operation telemetry when the display-oriented tool result is truncated or stored out of band.
  • Why it matters: The native review log can then show a successful curation with no operations, leaving callers unable to distinguish a clean no-review addition from missing review evidence.
  • What changed: Carry a bounded curation lifecycle result independently of display output, keep it internal to lifecycle hooks, validate the operation metadata, and persist an explicit verified or unresolved review-integrity state.
  • What did NOT change (scope boundary): This does not change curation policy, review approval/rejection behavior, model routing, or derived-artifact cleanup.

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Test
  • Chore (build, dependencies, CI)

Scope (select all touched areas)

  • TUI / REPL
  • Agent / Tools
  • LLM Providers
  • Server / Daemon
  • Shared (constants, types, transport events)
  • CLI Commands (oclif)
  • Hub / Connectors
  • Cloud Sync
  • CI/CD / Infra

Linked issues

  • None; this PR addresses a separate telemetry defect.

Root cause

  • Root cause: Structured curation operations shared the same result path as potentially large display output. Truncation happened before the lifecycle logger consumed that result, so review operations could disappear even though curation succeeded.
  • Why this was not caught earlier: Existing tests covered parsing normal tool results but not preservation across truncation or separation between internal lifecycle metadata and public streaming/history.

Test plan

  • Coverage added:
    • Unit test
    • Integration test
    • Manual verification only
  • Test files: focused agent/tool, transport, lifecycle-handler, storage, and result-parser tests in test/unit/.
  • Key scenarios: large code_exec output, direct curate output, malformed captures, high- and low-impact operations, internal-only transport, and persistence round trips.

User-visible changes

Curate log records can include reviewIntegrity: {status: "verified"} when structured operations were validated, or reviewIntegrity: {status: "unresolved", reason: "..."} when they were not. No CLI defaults or review policy changed.

Evidence

  • Failing test before + passing after
  • Trace/log snippets
  • Screenshot/recording

The added regression tests fail against the prior implementation because the internal lifecycle result and integrity state do not exist. They pass with this change.

Checklist

  • Tests added or updated and passing (npm test)
  • Lint passes (npm run lint)
  • Type check passes (npm run typecheck)
  • Build succeeds (npm run build)
  • Commits follow Conventional Commits format
  • Documentation updated (if applicable)
  • No breaking changes (or clearly documented above)
  • Branch is up to date with main

Risks and mitigations

  • Risk: Internal lifecycle metadata could leak into client streams or persisted task history.
    • Mitigation: Strip it at both public stream and router boundaries, with regression tests.
  • Risk: Malformed operation metadata could be treated as trustworthy.
    • Mitigation: Validate required review fields and persist an unresolved integrity state instead of silently accepting it.

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.

1 participant