Skip to content

Fix task-state availability and production diagnostics - #110

Open
sambitcreate wants to merge 6 commits into
mainfrom
fix/task-state-and-production-diagnostics
Open

Fix task-state availability and production diagnostics#110
sambitcreate wants to merge 6 commits into
mainfrom
fix/task-state-and-production-diagnostics

Conversation

@sambitcreate

Copy link
Copy Markdown
Owner

Summary

Fix the confirmed task-state and production-diagnostics defects found while investigating the desktop “Tasks unavailable” banner.

Separate fixes

  1. Authoritative task replay — validate the newest successful full task checkpoint, allowing it to supersede invalid ancestors while still rejecting an invalid newest checkpoint.
  2. Honest durable tracking — share task-state loading between chat-open and generation, omit the durable task tool for journalless turns, distinguish storage-disabled tracking from invalid snapshots, and validate tool output before mutation/publication.
  3. Rollout refresh and cohort preservation — reread device policy, serialize concurrent loads, publish complete initial JSON atomically, reject invalid/deleted replacements, and preserve the new-chat activation cutoff as stages widen.
  4. Privacy-preserving diagnostics — retain bounded structural causes/statuses, distinguish renderer exceptions from process crashes, count cancellations correctly, classify provider errors before outcome redaction, and record distinct task/MCP evidence without raw text.

Review process

Each fix received three independent opencode-go/deepseek-v4.1-flash adversarial reviews. Rollout and diagnostics received additional three-review rounds after fixing edge cases found during review. Review lenses covered correctness/integration, hostile inputs/races/privacy, and behavioral coverage.

Verification

  • npm test
  • npm run test:todo
  • npm run test:compaction
  • npm run test:diagnostics
  • Focused provider-failure and harness diagnostic tests
  • npm run type-check
  • npm run lint
  • npm run build
  • npm run test:e2e:diagnostics:production

Desktop todo IPC has no iOS/Android consumers. Portable provider-failure and shared-server contracts remain compatible.

Scope of the evidence

The old production logs discarded the causes of some provider, renderer, MCP and shutdown errors, so those historical causes remain unknown. This PR improves future diagnostic evidence. Production rollout advancement is a receipt-validated operator action; storage-disabled chats gain durable tracking when their cohort is enabled. These source changes reach the installed app through the normal release process.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ No critical issues — one minor robustness gap inline and one pre-existing observation.

Reviewed changes

  • Authoritative task replayreplayTodoState now scans the whole current Pi branch and validates only the newest non-isError todo result, so a newer valid checkpoint supersedes corrupt ancestors while a corrupt newest checkpoint still fails closed.
  • Honest durable tracking — chat-open and generation share loadDurableTodoSnapshot; journalless turns omit the durable tool and publish storage_not_enabled, corrupt replay publishes invalid_snapshot, and tool output is re-validated against the reader contract before generation-local mutation.
  • Rollout refresh and cohort preservationPiUpgradeRolloutStore.load() re-reads and validates policy on every call, serializes first reads, publishes initial JSON with a no-overwrite link, and advance() preserves activatedAt when widening past new_chats.
  • Privacy-preserving diagnostics — error projection traverses bounded cause chains through own descriptors only (no getters or stack), adds closed causeCode/httpStatus evidence, distinguishes renderer exceptions and cancellations from process crashes and failures, classifies provider failures before outcome redaction, and records content-free todo/MCP evidence.

ℹ️ Durable renderer referenceId never reaches the journal (pre-existing)

The renderer diagnostic event stores durableReferenceId as fields.referenceId, but the field normalizer redacts the UUID before the SAFE_REFERENCE check runs, so the value is silently dropped and production reports cannot be correlated to their journal evidence. This predates the PR — flagging it because this change is hardening exactly that event.

Technical details
# Durable renderer reference ids never persist

## Affected sites
- `main/handlers/diagnostics.ts:108` — generates `durableReferenceId = RD-${randomUUID()}`.
- `main/handlers/diagnostics.ts:120` — stores it as `fields.referenceId`.
- `main/services/diagnostics-contract.ts:181``UUID_VALUE` rewrites the UUID to `[REDACTED ID]` inside `sanitizeDiagnosticText`.
- `main/services/diagnostics-contract.ts:184,278``SAFE_REFERENCE` then rejects `RD-[REDACTED ID]`, so `normalizeDiagnosticFields` drops the field.

## Required outcome
- Either persist a correlatable durable reference or stop generating and displaying one.

## Suggested approach (optional)
- Validate `referenceId` against `SAFE_REFERENCE` before the generic scrubber, or exempt it from `UUID_VALUE`/`LONG_IDENTIFIER`.

## Verification
- `normalizeDiagnosticFields({ referenceId: "RD-e20a0162-266b-49ab-ae0a-078f74efe71c" })` returns `undefined`; `sanitizeDiagnosticText` yields `RD-[REDACTED ID]`.

## Open questions for the human (optional)
- Is this already tracked separately? It is not introduced by this PR.

ℹ️ Nitpicks

  • atomicPrivateJson(..., exclusive) now requires hard-link support for first policy creation, so a link-less userData volume (some SMB/exFAT or restricted mounts) would fail load() where the previous open(..., "wx") succeeded. Low risk for standard macOS/Windows/Linux userData and the atomicity win is real — noting it only so the tradeoff is explicit.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash𝕏

Comment thread main/services/pi-upgrade-rollout.ts
@sambitcreate

Copy link
Copy Markdown
Owner Author

Addressed both actionable Pullfrog observations: fc96b21 closes failed-advance policy observation, and 6f3bf77 preserves main-generated RD-UUIDv4 references through journal normalization with an exact structural grammar. Each follow-up received three fresh DeepSeek V4.1 Flash reviews (30 reviews total); full npm test, diagnostics/compaction suites, typecheck, lint and production diagnostics E2E passed. The hard-link requirement is an accepted atomic-publication tradeoff consistent with existing repository storage patterns. Please recheck the updated head.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Rollout observation latchPiUpgradeRolloutStore.readCurrent now marks the store as having observed a valid policy on every validated parse, so a failed advance() can no longer let a later load() silently recreate a deleted policy; the ENOENT initialization branch rechecks the latch after asynchronous work, and a deterministic overlapping-load regression test covers the sequence.
  • Durable renderer referencesnormalizeDiagnosticFields admits the exact RD-<UUIDv4> grammar for referenceId before the generic ID redactor, so main-generated durable references persist and correlate in the journal; the new test confirms newline/URL/Bearer-wrapped values still drop.

Pullfrog  | View workflow run | Using DeepSeek Flash𝕏

@sambitcreate

Copy link
Copy Markdown
Owner Author

Final verification on 6f3bf77: verify, Deterministic Electron E2E, changed-area detection and Pullfrog all succeeded; Android was correctly skipped. Pullfrog’s incremental review reports no new issues and its rollout thread is resolved. Both review follow-ups are separate commits, and 30 independent DeepSeek V4.1 Flash adversarial reviews are complete. Working tree clean; PR ready for merge.

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