Repository navigation
fix(publishing): fence synchronous provider effects (agent-harness#820) - #1683
ViperJuice wants to merge 5 commits into
Conversation
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. |
![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: c995127de5
ℹ️ 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".
|
|
||
| - Retained fence commit: `71c4ba73c1df8315e6fbe0d6fbc76546ad300bea`. Its parent is | ||
| `6e2b2a29`, an **unmerged, superseded** agent-harness#789 read-back retry. Main | ||
| landed a different #789 repair (`fb7c6b82`, `cfe8999d`). Take the fence only. |
There was a problem hiding this comment.
Qualify this reference as agent-harness#789; the plan also contains unqualified references for agent-harness#1577, agent-harness#807, agent-harness#1213, agent-harness#1550–agent-harness#1553, agent-harness#1576, and agent-harness#820. In this multi-repository fleet, those references are ambiguous and violate the repository-wide requirement that every issue or PR number name its repository.
AGENTS.md reference: AGENTS.md:L19-L29
Useful? React with 👍 / 👎.
| A drain between the two sections can leave a pushed branch without a new PR. | ||
| The second fence then refuses creation. The direct adapter can raise; if the |
There was a problem hiding this comment.
A drain that starts between the effects does not necessarily make the second fence refuse: resume_active() can restore ACTIVE with the same generation nonce after a refused rotation, allowing the still-valid lease and PR creation to proceed. State instead that creation is refused when the repository remains DRAINING at the second fence; otherwise this recovery runbook gives operators an incorrect guarantee about the resulting remote state.
Useful? React with 👍 / 👎.
…ess#1683) Combine require_current_generation from both sides: main's allow_draining (agent-harness#1633) is passed through to validate_lease inside #820's held section. An effect combined with allow_draining is refused before any lock, because a synchronous provider effect never runs under a DRAINING latch. No caller passes both. The validate_lease stub in the #820 lifetime test now takes allow_draining, and one test pins the combined behaviour. CHANGELOG.md and plans/manifest.json keep both sides' entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEJEudHgxc7NftvDdc8K6P
Refs agent-harness#820
What this changes
The publisher's synchronous provider effects (the push, and the PR create) now each run inside the generation fence.
require_current_generationholds the generation latch exclusively across lease validation, the active-generation-root check and the effect itself (live.py). Validation-only callers keep a null context.credsep.py, the push and the create are each wrapped in their own fenced call; the title probe sits outside both._revalidate_generation_before_effectreturnseffect()on the same predicates as before, and a declared lease still re-raises a namespace-resolution failure.plans/.Tests:
phase-loop-runtime/tests/test_publisher_effect_fence_820.py(31 tests), written to fail first (commit 473a5ce).Review record
The brokered board ran three rounds. In round 3 its president HELD (F001), because the codex red-team, Claude correctness and grok adversarial seats returned unusable output. With the owner's authorization, those three seats were then run by hand against the exact candidate checkout c995127 on base cba3aeb. All four lenses returned AGREE:
An Opus president then ruled MERGE-READY with no blocking findings, and recorded that the prior HOLD condition is satisfied. The president spot-checked the load-bearing claims against the checkout.
Evidence, in the authoring worktree on the dev host:
.phase-loop/diagnostics/effect-fence-820-reland/manual-panel/, holdingpresident-ruling.mdand the four seat reviews. The brokered rounds 1–3 are next to it.Verification, corrected
The mutation controls were stated earlier as "four failures each". The president measured them again in throwaway
git archivecopies (the candidate checkout was never written to):nullcontextin place of the exclusive latch)The acceptance criterion still holds: removing either fence fails the suite.
Still open, intentionally
agent-harness#820, agent-harness#833 and agent-harness#1628 stay open. Lease-holder liveness, the namespace-resolution bypass and the post-entry handoff are not resolved by this PR.
Non-blocking follow-ups, not part of this merge:
🤖 Generated with Claude Code
https://claude.ai/code/session_01PEJEudHgxc7NftvDdc8K6P
Need help on this PR? Tag
@codesmith-botwith what you need.