fix: preserve Linear publication evidence for recovery - #482
fix: preserve Linear publication evidence for recovery#482mldangelo-oai wants to merge 12 commits into
Conversation
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa3e7749ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa3e7749ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Please review the current head, |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed publication receipt verification, destination binding, event accounting, and recovery behavior.
|
@codex review Please review the current head |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mldangelo-oai
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review Please review exact head |
|
@codex security review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ecc7bc36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| events.unresolvedCompletions?.some( | ||
| (findingId) => !created.has(findingId), | ||
| ) === true || |
There was a problem hiding this comment.
When a completed tool result contains conflicting recognizable identities—for example, structured_content.identifier is SEC-1 while JSON text reports SEC-2—savedIssue classifies it as unresolved, but a model-written handoff selecting either value populates created, making this predicate false. Publication then accepts the selected identity and deletes the event evidence even though the trusted connector output was ambiguous; distinguish results with no recognizable identity from results with conflicting identities, and keep the latter indeterminate regardless of the handoff.
AGENTS.md reference: sdk/typescript/AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
kmbroai
left a comment
kmbroai
left a comment
There was a problem hiding this comment.
Critical review
Reviewed head f7ecc7bc36f5d020ccf1e5712030550e718ac11f, including the existing recovery comments, and compared #624.
Recommendation: do not merge this head independently; supersede it with the reconciled #624 implementation. Preserving evidence for uncertain mutations is necessary, but a model-written handoff can still erase a conflict in the connector's own result.
[P2] A handoff must not resolve contradictory connector identities by choosing one
I reproduced the current indeterminate predicate through publishScanInternal, not just the parser. The synthetic completed connector call had the exact expected mutation arguments, structured_content.identifier = SYNTH-1, and JSON text containing identifier = SYNTH-2. A matching handoff selected SYNTH-1.
At this head the operation returned success, passed SYNTH-1 to the persistence callback, and reported no indeterminate outcome. savedIssue collapses both “no recognizable identity” and “conflicting identities” to an unresolved completion; the handoff then fills created, making the unresolved-completion check false. This is the same unresolved issue described in the existing public review, independently confirmed at the current head.
Keep those states distinct. An unfamiliar result may be accounted for by an otherwise valid handoff, but contradictory recognized claims must remain uncertain regardless of which claim the handoff repeats. On the same fixture, #624 throws with an indeterminate receipt, records no success, and retains connector evidence.
Simplification and existing fixes
The shared exact mutation-argument builder and nonfatal event-log preservation are good changes. Earlier comments about auxiliary log failures, unfamiliar completed results, and success-shaped invalid handoffs have substantive fixes here; they should not all be reposted as unresolved. However, continuing to patch parallel created, failed, claimedIssues, and unresolvedCompletions collections makes the state distinctions fragile. Consolidate on #624's explicit evidence/reconciliation path instead of carrying both implementations.
Verification
Ran the three publication-event, integration, and publisher suites: 90 passed, 0 failed, plus the cross-head synthetic conflict reproduction. Linux, Bun 1.3.14 / Node 22.13.1 with cached dependencies. The reproduction used actual local handoff/reconciliation code and mocked connector/persistence boundaries; no Linear issue was created and no real credential was used.
3 participants
Summary
Preserve recovery evidence when a connected Linear publication completes but the SDK cannot verify every issue creation. Verified issues still reach local history and partial receipts. Unverified completions keep the private handoff and event evidence so users can inspect Linear before retrying.
Changes
main.Testing
f7ecc7bc, the seven-file publication suite passed 168 tests with 1,512 assertions and no failures.pnpm run typespnpm run formatpnpm packfollowed bypnpm run check:package: validated the build, 255 archive entries, 111 bundled plugin files, public imports, consumer types, CLI behavior, and the installed-package worker smoke.maincommits, the full seeded suite passed 1,567 tests with 30 expected skips, 21,548 assertions, and no failures across 86 files on87598157.git diff --check origin/main...HEADRisk and rollout
Default issue creation, the database schema, and credential handling are unchanged. This does not add automatic retries or remote readback. Exact argument verification fails closed, so an unexpected connector response is retained for manual recovery instead of being treated as a confirmed publication.
Recovery artifacts can contain issue descriptions and source evidence. They remain in private local state only when manual recovery is needed.
Public disclosure review
Automated comments already on this public pull request contain access-restricted report links, and three older commit objects retain their original corporate author identity. Those artifacts predate this update and remain visible in public history, so the second attestation stays open for maintainer review. New commits use the GitHub noreply identity.