Skip to content

fix: preserve OpenCode session evidence and snapshot accounting - #67354

Merged
pelikhan merged 6 commits into
mainfrom
pelikhan-opencode-session-audit
Oct 10, 2026
Merged

pelikhan merged 6 commits into
mainfrom
pelikhan-opencode-session-audit

Conversation

@pelikhan

@pelikhan pelikhan commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Preserve attributed stream errors as standard session.error observations without inventing terminal outcomes, usage, or turns. Reject quoted, fenced, unrelated, and malformed diagnostics.
  • Reconcile message/tool snapshots into one canonical observation while retaining native revisions, exact content, partial markers, structured results, and previously unavailable tool inputs.
  • Reconcile revised step accounting once, consistently mark overflowing token totals unavailable, and omit overflowing cost subtotals.
  • Map explicit refusal/content-filter signals to assistant.refusal, without classifying ordinary disclaimers or tool payloads as refusals.
  • Add sanitized native-run fixtures, persistence regressions, and OpenCode-specific documentation.

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 as 24247c125b. 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-cjs and make lint-cjs
  • make agent-report-progress and corrective make agent-report-progress-no-test
  • npm run typecheck --silent
  • Initial OpenCode/parser/persistence integration selection: 178 tests passed
  • After prerequisite integration, the exact previously failing OpenCode suite passed all 39 tests without an external preload; related session checks passed 829 tests
  • Session schema freshness, formatting, lint, and corrective build checks passed after integration
  • Syntax checks and actual-artifact canonical/unified replay

Impacted 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.

pelikhan and others added 2 commits October 9, 2026 22:33
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>
@pelikhan
pelikhan marked this pull request as ready for review October 10, 2026 05:44
Copilot AI balanced review requested due to automatic review settings October 10, 2026 05:44
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

✅ Ponytail Reviewer completed successfully!

Generated by Ponytail Reviewer for #67354

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff

🧪 Test quality analysis by Test Quality Sentinel

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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).

🏗️ ADR gate enforced by Design Decision Gate 🏗️

@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

✅ PR Code Quality Reviewer completed the code quality review.

No GitHub write emitted yet while I resolve safeoutputs invocation constraints.

🔎 Code quality review by PR Code Quality Reviewer

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread docs/src/content/docs/reference/third-party-agent.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 thread actions/setup/js/parse_opencode_log.cjs
@github-actions github-actions Bot mentioned this pull request Oct 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)
reviewed_at: 2026-10-10T05:47:32Z
review_event: REQUEST_CHANGES
top_themes:
  - unstable canonical identity for revised message snapshots
  - unstable canonical identity for revised tool completions
  - backward-only refusal reconciliation on out-of-order records
files_reviewed:
  - actions/setup/js/parse_opencode_log.cjs
  - actions/setup/js/parse_opencode_log.test.cjs
  - actions/setup/js/fixtures/opencode_ci_sessions.cjs
  - docs/src/content/docs/reference/third-party-agent.md
comment_count: 3

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 61.9 AIC · ⌖ 5.83 AIC · ⊞ 20K · ◷
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 themesThese 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

Comment thread actions/setup/js/parse_opencode_log.cjs Outdated
Comment thread actions/setup/js/parse_opencode_log.cjs Outdated
Comment thread actions/setup/js/parse_opencode_log.cjs Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 in nativeSnapshots while collapsing to one canonical observation.
  • Sticky failure/refusal latching (success === false once 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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 real stream error line 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 reports success: false, the merged error/metadata/exitCode fields 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/accumulateSessionUsage overflow handling is careful and well-tested (verified independently via direct script execution against agent_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 getMessageRefusal is 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

Comment thread actions/setup/js/parse_opencode_log.cjs
Comment thread actions/setup/js/parse_opencode_log.cjs Outdated
Comment thread actions/setup/js/parse_opencode_log.test.cjs
@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.
  2. Review (docs/src/content/docs/reference/third-party-agent.md:182): 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. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  3. Review (actions/setup/js/parse_opencode_log.cjs:187): result is retained only in the parser output. The unified writer maps tool.execution_complete.output from the aliases ["output", "result"] (unified_session_payload.cjs:143), so when a snapshot supplies both fields, it selects output and silently drops this structured result. That violates T-UAS-013's requirement that both coexist and means the new persistence path does not preserve this evidence. Please model result as a distinct unified field and cover the coexistence case through writeUnifiedSession. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  4. Review (actions/setup/js/parse_opencode_log.cjs:178): Tool completion reconciliation is using part.id instead of the stable callID, so a revised snapshot can still fork into a second tool.execution_complete event and double-count the same logical tool call. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  5. Review (actions/setup/js/parse_opencode_log.cjs:149): This message reconciliation still keys revisions off part.id, so the same logical assistant message will be emitted twice if OpenCode replays it under the same messageID with a fresh part ID. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  6. Review (actions/setup/js/parse_opencode_log.cjs:222): This refusal mapping only rewrites text parts that have already been seen, so a step_finish refusal that arrives before its matching text will never be reconciled and can leave duplicate or content-less refusal events behind. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  7. Review (actions/setup/js/parse_opencode_log.cjs:193): [/diagnosing-bugs] Sticky-failure merge never clears a stale error field: completion only overwrites error when state.error !== undefined, so once a part is observed with success: false, the spread {...previous?.event.data, ...completion} keeps the old error text forever, even if a later authoritative snapshot of the same part legitimately drops the error. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  8. Review (actions/setup/js/parse_opencode_log.cjs:82): [/diagnosing-bugs] The fence toggle is naive run-length state that never resets, so a single unterminated/truncated code fence (common when logs are cut mid-stream) permanently flips fenced = true for the remainder of the file — silently dropping every subsequent real stream error logfmt line as if it were quoted prose. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)
  9. Review (actions/setup/js/parse_opencode_log.test.cjs:236): [/tdd] The new test suite (186 lines) is thorough for the happy paths but has a gap directly tied to this PR's own thesis ("preserve evidence even from partial/truncated logs"): there's no test for an unterminated fence at EOF, which — as noted in the sibling comment on line 82 — silently drops real session.error evidence. Given the PR explicitly targets partial-log robustness, this edge case deserves a regression test alongside the existing quoted/fenced/malformed cases in the "does not reinterpret diagnostic prose..." test. - fix: preserve OpenCode session evidence and snapshot accounting #67354 (comment)

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
Sous-chef work: 00c90a9a7ba129b4c2e89b45a8ee4e34a436cf60048e3dc721368b2eac3e09a7 22ee464c487c43c1de72598bd2a4797bb64fbc84699a4f1db5aaf0855eb6e766 90b12b7670ecdedc67c19dd1cd13b6de090ac28bafd4720776277d7f5d4bd7dc a4b09d19d8aa8d43c4f032b83c146e3cf6e366bd53832a661d4b0b30e0116096 b55987544e31a8914d7cd4d667325a5f26ede4655f966cfdcbc03255343bbc57 be4ccb33ba8df033f6f1fc0c3d5e8457e08778c1e88a2bbc3f5e31f31c4aecee cc148019d3f84602a5e0b960470424f59e561aedcee63d26e3c7dbb3208acf79 f4fbe8d26fa2ee965626b456e1a566474c24f9f791bfe34e64ed097676c41ff4
Sous-chef state: 4e5c5969ba74bd7520f21986b15edae3991fe37e1dff652b30645fee31c9d3ac

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 8.49 AIC · ⌖ 8.78 AIC · ⊞ 1K · ◷
Comment /souschef to run again

Copilot AI and others added 2 commits October 10, 2026 06:49
…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>
@github-actions

Copy link
Copy Markdown
Contributor

Great work @pelikhan — the OpenCode evidence/snapshot accounting fix is focused and well tested. Looks ready for review.

Generated by ✅ Contribution Check · copilot · auto · 41.5 AIC · ⌖ 0.668 AIC · ⊞ 9.2K · ◷

@gh-aw-bot

Copy link
Copy Markdown
Collaborator

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes.

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
Sous-chef work:
Sous-chef state: 3c3c2bc5c39608c3b3d85a7c963966de2f252e9eaaf2ecbf7f3f42cb23b6fcbc

Generated by 👨‍🍳 PR Sous Chef · pi · haiku45 · 3.82 AIC · ⌖ 9.7 AIC · ⊞ 1K · ◷
Comment /souschef to run again

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>
@pelikhan
pelikhan merged commit 5496d8d into main Oct 10, 2026
14 of 15 checks passed
@pelikhan
pelikhan deleted the pelikhan-opencode-session-audit branch October 10, 2026 10:36

Copilot AI commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

@copilot address the following outstanding work in one pass:

  1. Update this branch with the latest main using make merge-main, resolving any conflicts and preserving the intended changes....

Ran make merge-main and integrated the latest main locally as merge commit e4193ab; build, workflow compilation, formatting, and impacted tests passed. The progress push was rejected because the remote disallowed creating the branch ref. The PR was then merged by @pelikhan at df05bb4 before I could publish e4193ab, so that additional merge commit remains local and was not pushed.

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.

4 participants