Skip to content

fix(harness): reconcile uncertain Assistant Resume commits [SAP-3292] - #1024

Closed
ynadge wants to merge 1 commit into
assistant-compaction-provenancefrom
assistant-resume-commit-reconciliation
Closed

ynadge wants to merge 1 commit into
assistant-compaction-provenancefrom
assistant-resume-commit-reconciliation

Conversation

@ynadge

@ynadge ynadge commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Primary change type

  • Bug fix

Problem and motivation

A failed metadata acknowledgement could be mistaken for a completed rollback, and private Resume replay accepted a changed execution argument.

Summary and scope

Report an explicit unconfirmed commit when publication succeeded but durability/rollback did not. Retain both valid generations for reconciliation. Reject original-base Resume replay with different execution, while exact lost-ACK proof still returns the committed lifecycle.

Related work

Related issue: https://linear.app/sapiom/issue/SAP-3292

Depends on #1023. Native stack target: main.

Validation

45 focused tests passed across Resume, Resume-store and metadata files. Full Harness server TypeScript, scoped ESLint and diff checks passed.

Tests and documentation

Inject both post-publication durability failure and rollback failure, then restart and reconcile the exact Resume UUID/revision. Verify prior generation retention and execution-mismatch rejection. Updated assistant-history.md. Increment:69 changed lines including tests/docs/changeset; SDK pr:size is absent.

Compatibility and release impact

  • Failure does not promise rollback. Existing public error handling remains safe; the new private code makes commit ambiguity explicit.
  • Changeset: @sapiom/harness patch.
  • Fix-forward: reconcile the exact durable operation proof instead of allocating or assuming rollback after uncertain IO.

Security

  • No secrets, credentials, private data, or unsanitized logs included.
  • No suspected vulnerability disclosed publicly.

AI assistance

  • AI assistance used: Codex implemented and reviewed the change with the validation described above.

Checklist

  • CONTRIBUTING.md read; maintainer-authorized issue work.
  • One focused increment, checks, documentation and release impact addressed.
  • I can explain and maintain the submitted changes.

Devin Review

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4e00fd44-0709-49a8-a226-908f85cbd025

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration 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.

Devin Review found 1 potential issue.

Devin Review

Comment on lines +134 to +135
if (published && !restored)
throw new AssistantStorageCommitUnconfirmedError();

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.

🟡 Cleanup masks unconfirmed commits

When temporary-file removal fails after uncertain publication, writeAssistantJson replaces AssistantStorageCommitUnconfirmedError with the cleanup error. Callers cannot identify and reconcile the possibly committed operation.

Learn more

A rejection from an awaited finally block replaces the error already thrown by its try or catch. Here, storage has already published the destination and failed to confirm rollback. Another filesystem failure during fs.rm therefore removes the only signal that the commit is uncertain. This is especially reachable during the same degraded-storage condition that caused publication or rollback to fail.

Example: The destination rename succeeds, directory sync fails with EIO, rollback also fails, and removing .rollback then fails with EIO. The caller receives the removal error without ASSISTANT_STORAGE_COMMIT_UNCONFIRMED, although the new generation remains readable.

Recommended fix: Make temporary cleanup best-effort when an earlier error is pending, or capture cleanup failures without replacing AssistantStorageCommitUnconfirmedError. Preserve cleanup failures only when no primary operation error exists.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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