Repository navigation
fix: preserve OpenCode session evidence and snapshot accounting - #67354
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep complete native logfmt evidence in the canonical trace while checking the shared essential unified payload contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #67354 does not have the implementation label and has 0 new lines of code in business logic directories (4 files changed, threshold 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review. No GitHub write emitted yet while I resolve safeoutputs invocation constraints.
|
There was a problem hiding this comment.
L182: shrink: 24-line artifact-history narrative. Keep one sentence linking the fixture; provenance and historical accounting belong in fixture comments or the PR description.
net: -23 lines possible.
Generated by ✂️ Ponytail Reviewer for #67354 · codex · gpt56 · 11.9 AIC · ⌖ 6.32 AIC · ⊞ 13.5K
Comment /ponytail to run again
There was a problem hiding this comment.
🟡 Changes recommended
Unified persistence drops structured tool results whenever output coexists, undermining evidence preservation.
1 open finding
What changed in this PR
Improves OpenCode session parsing to preserve native errors, revised snapshots, refusals, and accurate accounting.
Changes:
- Reconciles message, tool, and accounting snapshots.
- Adds stream-error recovery and refusal handling.
- Adds regression fixtures, tests, and documentation.
| File | Description |
|---|---|
actions/setup/js/parse_opencode_log.cjs |
Implements enhanced OpenCode normalization. |
actions/setup/js/parse_opencode_log.test.cjs |
Adds parser and persistence regressions. |
actions/setup/js/fixtures/opencode_ci_sessions.cjs |
Provides sanitized native-run fixtures. |
docs/src/content/docs/reference/third-party-agent.md |
Documents OpenCode session evidence behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Request changes
The parser still has three reconciliation gaps in the new OpenCode snapshot logic: revised messages are keyed by part.id instead of a stable message identity, revised tool completions are keyed by part.id instead of callID, and refusal finish signals only merge backward into already-seen text.
Blocking themes
These all matter in the exact cases this patch is trying to harden: revised snapshots and out-of-order native records. In those shapes the canonical session can still duplicate assistant/tool observations or publish a refusal without its later content, which puts the normalized trace and accounting back out of sync again.🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 61.9 AIC · ⌖ 5.83 AIC · ⊞ 20K
Comment /review to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Reviewed the OpenCode session parser changes (parse_opencode_log.cjs) focusing on correctness/reliability of the new logic:
- Stream-error logfmt parsing (
parseOpenCodeStreamError): regex-based field extraction is bounded (no catastrophic backtracking), rejects malformed/fenced/unrelated diagnostics correctly, and doesn't fabricate terminal outcomes — matches the accompanying tests. - Snapshot reconciliation (
updatePartSnapshot, tool/text/reasoning dedup): part-identity tracking correctly retains native revisions innativeSnapshotswhile collapsing to one canonical observation. - Sticky failure/refusal latching (
success === falseonce set stays false; refusal classification persists across snapshot updates): intentional and reasonable — prevents a correction from silently "un-failing" a tool call or "un-refusing" a message once explicit evidence was seen. - Usage/cost reconciliation (
reconcileSessionUsage, overflow tracking, cost summation per step): traced through overflow propagation and confirmed previously-overflowed fields stay marked unavailable across updates, and overflowing cost sums are omitted rather than partially reported.
No blocking correctness, security, or missing-edge-case issues found in the changed lines. The existing automated comment on docs verbosity (L182) is still valid and doesn't need duplication here.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 73.2 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one real evidence-loss bug plus a matching coverage gap; the rest of the change is solid.
📋 Key Themes & Highlights
Key Themes
- Truncated-fence evidence loss (
parse_opencode_log.cjs:82):openCodeRecords's fence toggle never resets on EOF, so one unterminated code fence (the exact failure mode a truncated/partial log produces) permanently suppresses every subsequent realstream errorline for the rest of the file. This directly contradicts the PR's stated goal of preserving evidence from partial logs. - Stale sticky error text (
parse_opencode_log.cjs:193): once a tool part observation reportssuccess: false, the mergederror/metadata/exitCodefields from that snapshot can outlive a later, legitimate snapshot that no longer reports an error — the newer completion keeps showing the old error string. Worth an explicit comment if intentional, or a fix if not. - Test gap: no regression test exercises the truncated-fence scenario alongside the existing quoted/fenced/malformed logfmt cases.
Positive Highlights
- ✅ Excellent fixture provenance: sanitized real CI artifacts with clear traceability and explicit evidence-limits documentation (not overclaiming conformance).
- ✅
reconcileSessionUsage/accumulateSessionUsageoverflow handling is careful and well-tested (verified independently via direct script execution againstagent_session.cjs). - ✅ Scope discipline — shared session helpers, types, and other engine parsers are untouched; changes are isolated to OpenCode.
- ✅ Refusal classification correctly restricted to structured provider signals (verified
getMessageRefusalis not triggered by ordinary disclaimers).
Validated independently via direct Node execution against the real parser and fixtures (vitest wasn't installable in this sandbox due to a blocked registry) — the two issues above reproduce as described; all other traced paths (step reconciliation, snapshot dedup, overflow accounting) behaved as the new tests assert.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 146.6 AIC · ⌖ 14.7 AIC · ⊞ 10.1K
Comment /matt to run again
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 24247c1
|
…sion-audit Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
Great work @pelikhan — the OpenCode evidence/snapshot accounting fix is focused and well tested. Looks ready for review.
|
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: c1ff652
|
Integrate Claude and Goose session audit updates while preserving distinct tool output/result fields and nullable exit-code coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ran |

OpenCode can report provider stream errors only in stderr logfmt, leaving the canonical agent trace empty even when native error evidence exists. Part-ID-only deduplication also discarded changed snapshots, while accounting overflow could leave misleading partial totals.
Changes
session.errorobservations without inventing terminal outcomes, usage, or turns. Reject quoted, fenced, unrelated, and malformed diagnostics.assistant.refusal, without classifying ordinary disclaimers or tool payloads as refusals.Shared-contract prerequisite
The OpenCode assertion correction exposed a missing shared dependency in JS Tests (shard 1/4), not an infrastructure failure. With the shared-contract owner's explicit authorization, this branch now includes published common prerequisite
b39ff126e9, cherry-picked intact as24247c125b. This integrates the essential unified-session projection and related shared contract changes rather than creating an OpenCode-specific projection or weakening the assertions. Original OpenCode commits remain separately identifiable. Canonical errors retain all native fields; unified errors retain the exact essential payload and provenance timestamp.Evidence and limits
Read-only review used existing October 5 artifacts from a successful agent followed by failed detection, and October 9 artifacts containing logfmt-only provider errors. Local replay preserves the October 5 session's reported 255,330 tokens and recovers two October 9 error observations without fabricated accounting. Historical artifacts were not rewritten and no workflow runs were triggered.
The committed excerpt contains two observed step reports, not the full run's twelve turns. Snapshot, refusal, reasoning, and overflow cases are supplemental synthetic coverage; the sampled native JSON session did not expose reasoning or refusals. Historical bash-exit misclassification was already corrected in the baseline parser and is covered here as a regression, not a new fix.
Validation
make fmt-cjsandmake lint-cjsmake agent-report-progressand correctivemake agent-report-progress-no-testnpm run typecheck --silentImpacted Go tests were skipped because no Go files changed. One pre-existing empty-catch lint warning remains; no new lint warnings were introduced. No tests were skipped or assertions weakened to resolve the CI failure.