Skip to content

test(goal): CI-verify @muhammad-luay #276 stale-write fix on latest main (#275) - #288

Closed
ByBrawe wants to merge 1 commit into
mainfrom
test/contributor-276-fresh-writer-verified-ci-20261027
Closed

ByBrawe wants to merge 1 commit into
mainfrom
test/contributor-276-fresh-writer-verified-ci-20261027

Conversation

@ByBrawe

@ByBrawe ByBrawe commented Oct 27, 2026

Copy link
Copy Markdown
Owner

Verification-only mirror of the original contribution

Original contributor: @muhammad-luay, PR #276. Thank you for the substantial V1 and V2 concurrency work. Its 11 fork-origin Actions checks are still action_required rather than passed. To get reproducible owner-repository CI evidence, this draft PR reproduces exactly the 18 changed source/test blobs from fork commit 6e988e5 as a single test commit on top of current Goal main de15109 (includes verified closure-proof #286 and native OpenTUI peer #287). Each source blob SHA was checked identical. No workflows, package scripts or secrets were copied from the fork.

Questions this CI must settle

  • V1 foreground stale-write recovery preserves changes and rejects same-session Goal/revision replacement.
  • Native V2 work tools retry only stale generation and protect against changed progress, TODO, checks, file evidence or requirements during semantic completion.
  • Non-stale errors and read-only/owner boundary fail closed; reentrant callbacks do not duplicate external side effects.
  • All Windows/Linux test, release readiness, exact 2.1.4 native host, Loop coexistence, restart, and native Goal sidebar workflows pass.
  • The new #286 goal completion proof and #287 OpenTUI 0.5/0.7 strict compatibility are preserved, never reverted.

Safety and credit

Do not merge this draft automatically. If all gates pass, review and reconcile with original contributor PR #276 to preserve attribution and avoid duplicate deployment. If any fails, add deterministic failing fixture and work with the contributor on a targeted fix. This owner-side mirror solely enables trustworthy CI evidence, not a publish or claim that #275 is fully resolved. Maintainer reviewer notes are in #276; original PR remains open.

Co-authored-by: muhammad-luay muhammad-luay@users.noreply.github.com

…current main

This maintenance mirror preserves exact contributor source/test blob hashes from muhammad-luay/opencode-goal@6e988e5, against Goal main including #286 and #287, without importing fork CI workflows. This is a *test branch*, not a PR merge or published release.

Co-authored-by: muhammad-luay <muhammad-luay@users.noreply.github.com>
@ByBrawe
ByBrawe marked this pull request as ready for review October 27, 2026 19:34
Copilot AI balanced review requested due to automatic review settings October 27, 2026 19:34
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T19:38:54.236313Z 396e74e Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

ByBrawe pushed a commit that referenced this pull request Oct 27, 2026
…Code 2 (#275 #276)

Contributed by @muhammad-luay. Keep V1/2 state writers on fresh snapshots and retry stale generation only, with bounded 1..8 attempts. Guard same-session Goal/revision replacement; re-evaluate changed verification evidence, progress fingerprints, Todo, declared files and requirements before completion; ensure async file evidence is refreshed on collision. Fully tested in owner-repository verification mirror #288 against latest main, all 11 workflows green. Published package and reporter replay remain tracked in #275.

Co-authored-by: Muhammad Luay <muhammadluay45@gmail.com>

ByBrawe commented Oct 27, 2026

Copy link
Copy Markdown
Owner Author

CI mirror completed successfully: 18 fork source/test blobs from @muhammad-luay's #276 were SHA-identical, and all 11 owner CI workflows passed on latest main (de15109) with Windows/Linux, native 2.1.4, restart, real Loop coexistence and strict package readiness. The original contributor PR #276 was then approved and merged directly to main as 478c28a, preserving its proper attribution; this mirror must not be merged again. Closing only the duplicate verification PR. Thank you @muhammad-luay.

@ByBrawe ByBrawe closed this Oct 27, 2026

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 396e74e7c6

ℹ️ 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".

const owned = textFromParts(output.parts)
const next = replaceGoalConstraints(current, parsed.constraints)
await store.save(next)
const result = await mutateGoal(store, event.sessionID, (current) => replaceGoalConstraints(current, parsed.constraints))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Guard constraint retries against revision changes

When another process edits the Goal after this mutator's first load but before its save, mutateGoal retries against the new revision and applies the older command's constraint flags to it. This silently produces a Goal whose newer objective/requirements are paired with constraints from the superseded /goal create or /goal edit; capture the post-command Goal ID/revision on the first attempt and reject subsequent attempts when either changes.

Useful? React with 👍 / 👎.

Comment thread src/opencode/recovery.ts
Comment on lines +220 to +223
await mutateGoal(store, sessionID, (latest) => {
if (latest.id !== current.id || latest.revision !== current.revision || latest.status !== "active") return null
return pauseGoal(latest, restrictedAgentStopReason(currentAgent))
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Recheck the execution agent before pausing recovery

If a concurrent writer changes the execution agent from a restricted agent to Build after currentAgent is read, the retry still pauses the fresh active snapshot because the guard checks only ID, revision, and status. Execution-context updates do not increment the Goal revision, so startup recovery can incorrectly pause a Goal that is now eligible to run; make the restricted-agent decision from latest.execution?.agent and reconsider the outer recovery path when it changed.

Useful? React with 👍 / 👎.

Comment thread src/persistence/store.ts
@@ -615,11 +635,11 @@ export async function mutateGoal(
return { goal: next, wrote: true }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Report semantic no-ops as unwritten

When a mutator returns a state that differs only by updatedAt, GoalStore.save() deliberately skips the write and preserves the generation, but this helper still returns wrote: true. Callers use that flag to authorize external side effects: for example, two recovery coordinators can both observe a due retry, the second can re-mark an already-dispatched nextRetryAt: 0 state as a semantic no-op, and wakeHostLimitRetry will nevertheless schedule a second autonomous continuation. Derive wrote from whether persistence actually advanced rather than merely whether save() returned.

Useful? React with 👍 / 👎.

Comment on lines +2225 to +2226
if (!observed) continue
observedGoal = observed.goal

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Stop stale terminal events after a rejected mutation

If the Goal is edited between coordinatorGoal() and this mutation, the ID/revision guard returns null, but mutateGoal represents that as a truthy { goal, wrote: false }, so this check does not stop the old execution-terminal event. The handler then treats the newer revision as successfully observed, marks the old execution successful, and can prepare or dispatch continuation for the new revision; verify that observed.goal still matches the captured Goal before continuing. The later unitResult truthiness check has the same stale-revision fall-through.

Useful? React with 👍 / 👎.

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.

2 participants