[codex] add forward-only accepted publications - #39
Open
divo12 wants to merge 1 commit into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ofw/evolution/candidate_git.py">
<violation number="1" location="src/ofw/evolution/candidate_git.py:66">
P2: If the accepted ref advances during `prepare`, the second `_accepted_source` lookup can create a worktree from a commit different from the validated hypothesis source. Resolve the source once, validate that value, and use the same value for worktree creation.</violation>
<violation number="2" location="src/ofw/evolution/candidate_git.py:167">
P2: When the evolution ledger is corrupt, busy, or unreadable, `_accepted_source` lets `EvolutionLedgerFailure` escape the candidate API. Catch `EvolutionLedgerFailure` here and map it to `CandidateFailure`, matching `PublicationService`'s failure boundary.</violation>
</file>
<file name="src/ofw/evolution/ledger.py">
<violation number="1" location="src/ofw/evolution/ledger.py:636">
P1: When a payload contains any newly added publication field, this fallback still validates the digest after stripping that field. A modified or injected `content_tree`, `parent_release_id`, or operation metadata can therefore pass ledger validation with an old PR4 digest; allow the legacy digest only when all new fields retain their legacy defaults.</violation>
</file>
<file name="src/ofw/evolution/controller.py">
<violation number="1" location="src/ofw/evolution/controller.py:400">
P1: When the completion event write fails after the ref CAS, this preflight makes the controller return `PUBLICATION_FAILED` on every retry instead of letting `PublicationService.promote` resume the durable intent. Let the publication service inspect and resume an existing operation before requiring a readable current publication.</violation>
<violation number="2" location="src/ofw/evolution/controller.py:409">
P1: When a caller retries a completed `PUBLISH` request, the controller does not replay it because publication events are keyed by the request digest, not `request_id`. Record a controller-level completion or make request replay recognize the publication operation before advancing the next phase.</violation>
<violation number="3" location="src/ofw/evolution/controller.py:416">
P1: During `PUBLISH`, any recomputed ACCEPT decision for the current policy can be used even when it differs from the decision that produced `CandidateAccepted`. Require `promotion_decision.decision_id` to match `state.decision_id` before calling `promote`.</violation>
</file>
<file name="src/ofw/evolution/publication.py">
<violation number="1" location="src/ofw/evolution/publication.py:287">
P2: When the matching operation completed as a rollback, `promote` returns it as a successful publication. Require a `ReleasePublished` completion in this branch before calling `_completed`.</violation>
<violation number="2" location="src/ofw/evolution/publication.py:641">
P1: When recovery or an intent retry supplies a different valid policy digest, the service writes that caller value into the completion and can brick the accepted publication for the actual policy. Validate the non-null intent digest before completing, or use the durable intent digest for the completion.</violation>
<violation number="3" location="src/ofw/evolution/publication.py:850">
P1: When an earlier publication operation has durable intent but no completion, a different operation can reserve and publish the same `publication_id`. Include pending publication-intent targets in the collision check to preserve publication identity uniqueness.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
Comment on lines
+636
to
+639
| return digest in { | ||
| _digest(_payload_json(payload, legacy=False)), | ||
| _digest(_payload_json(payload, legacy=True)), | ||
| } |
There was a problem hiding this comment.
P1: When a payload contains any newly added publication field, this fallback still validates the digest after stripping that field. A modified or injected content_tree, parent_release_id, or operation metadata can therefore pass ledger validation with an old PR4 digest; allow the legacy digest only when all new fields retain their legacy defaults.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/ledger.py, line 636:
<comment>When a payload contains any newly added publication field, this fallback still validates the digest after stripping that field. A modified or injected `content_tree`, `parent_release_id`, or operation metadata can therefore pass ledger validation with an old PR4 digest; allow the legacy digest only when all new fields retain their legacy defaults.</comment>
<file context>
@@ -605,6 +626,45 @@ def _event_identity(event: EvolutionEvent) -> str:
+
+
+def _payload_digest_matches(payload: EvolutionEventPayload, digest: str) -> bool:
+ return digest in {
+ _digest(_payload_json(payload, legacy=False)),
+ _digest(_payload_json(payload, legacy=True)),
</file context>
Suggested change
| return digest in { | |
| _digest(_payload_json(payload, legacy=False)), | |
| _digest(_payload_json(payload, legacy=True)), | |
| } | |
| current = _digest(_payload_json(payload, legacy=False)) | |
| if digest == current: | |
| return True | |
| if isinstance(payload, (ReleasePublished, ReleaseRolledBack)): | |
| if any( | |
| value is not None | |
| for value in ( | |
| payload.content_tree, | |
| payload.parent_release_id, | |
| payload.expected_current_commit, | |
| payload.policy_digest, | |
| payload.operation_id, | |
| payload.intent_event_id, | |
| ) | |
| ): | |
| return False | |
| elif isinstance(payload, ExternalOperationIntent): | |
| if payload.target_reached or any( | |
| value is not None | |
| for value in ( | |
| payload.expected_current_commit, | |
| payload.candidate_commit, | |
| payload.content_tree, | |
| payload.policy_digest, | |
| payload.parent_release_id, | |
| payload.target_release_id, | |
| ) | |
| ): | |
| return False | |
| else: | |
| return False | |
| return digest == _digest(_payload_json(payload, legacy=True)) |
| root=self._workspace_root, | ||
| experiment_id=request.experiment_id, | ||
| policy_digest=current.policy_digest, | ||
| operation_id=request.digest(), |
There was a problem hiding this comment.
P1: When a caller retries a completed PUBLISH request, the controller does not replay it because publication events are keyed by the request digest, not request_id. Record a controller-level completion or make request replay recognize the publication operation before advancing the next phase.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/controller.py, line 409:
<comment>When a caller retries a completed `PUBLISH` request, the controller does not replay it because publication events are keyed by the request digest, not `request_id`. Record a controller-level completion or make request replay recognize the publication operation before advancing the next phase.</comment>
<file context>
@@ -371,12 +375,53 @@ def _advance_waiting(
+ root=self._workspace_root,
+ experiment_id=request.experiment_id,
+ policy_digest=current.policy_digest,
+ operation_id=request.digest(),
+ publication_id=request.release_id,
+ expected=current.cas_token,
</file context>
| candidate_tree=self._publication.commit_tree( | ||
| self._workspace_root, state.candidate_commit | ||
| ), | ||
| gate=request.promotion_decision, |
There was a problem hiding this comment.
P1: During PUBLISH, any recomputed ACCEPT decision for the current policy can be used even when it differs from the decision that produced CandidateAccepted. Require promotion_decision.decision_id to match state.decision_id before calling promote.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/controller.py, line 416:
<comment>During `PUBLISH`, any recomputed ACCEPT decision for the current policy can be used even when it differs from the decision that produced `CandidateAccepted`. Require `promotion_decision.decision_id` to match `state.decision_id` before calling `promote`.</comment>
<file context>
@@ -371,12 +375,53 @@ def _advance_waiting(
+ candidate_tree=self._publication.commit_tree(
+ self._workspace_root, state.candidate_commit
+ ),
+ gate=request.promotion_decision,
+ )
+ except PublicationFailure:
</file context>
| ) | ||
| try: | ||
| policy = self._policy(request.experiment_id) | ||
| current = self._publication.current_accepted( |
There was a problem hiding this comment.
P1: When the completion event write fails after the ref CAS, this preflight makes the controller return PUBLICATION_FAILED on every retry instead of letting PublicationService.promote resume the durable intent. Let the publication service inspect and resume an existing operation before requiring a readable current publication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/controller.py, line 400:
<comment>When the completion event write fails after the ref CAS, this preflight makes the controller return `PUBLICATION_FAILED` on every retry instead of letting `PublicationService.promote` resume the durable intent. Let the publication service inspect and resume an existing operation before requiring a readable current publication.</comment>
<file context>
@@ -371,12 +375,53 @@ def _advance_waiting(
+ )
+ try:
+ policy = self._policy(request.experiment_id)
+ current = self._publication.current_accepted(
+ self._workspace_root,
+ request.experiment_id,
</file context>
| payload.candidate_commit or "", | ||
| ) | ||
|
|
||
| def _append_published_from_intent( |
There was a problem hiding this comment.
P1: When recovery or an intent retry supplies a different valid policy digest, the service writes that caller value into the completion and can brick the accepted publication for the actual policy. Validate the non-null intent digest before completing, or use the durable intent digest for the completion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/publication.py, line 641:
<comment>When recovery or an intent retry supplies a different valid policy digest, the service writes that caller value into the completion and can brick the accepted publication for the actual policy. Validate the non-null intent digest before completing, or use the durable intent digest for the completion.</comment>
<file context>
@@ -0,0 +1,1059 @@
+ payload.candidate_commit or "",
+ )
+
+ def _append_published_from_intent(
+ self, root: Path, experiment_id: str, policy_digest: str, intent: EvolutionEvent
+ ) -> PublishedPublication:
</file context>
|
|
||
|
|
||
| def _check_publication_collision(events: tuple[EvolutionEvent, ...], publication_id: str) -> None: | ||
| if any(_publication_id(event) == publication_id for event in events): |
There was a problem hiding this comment.
P1: When an earlier publication operation has durable intent but no completion, a different operation can reserve and publish the same publication_id. Include pending publication-intent targets in the collision check to preserve publication identity uniqueness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/publication.py, line 850:
<comment>When an earlier publication operation has durable intent but no completion, a different operation can reserve and publish the same `publication_id`. Include pending publication-intent targets in the collision check to preserve publication identity uniqueness.</comment>
<file context>
@@ -0,0 +1,1059 @@
+
+
+def _check_publication_collision(events: tuple[EvolutionEvent, ...], publication_id: str) -> None:
+ if any(_publication_id(event) == publication_id for event in events):
+ raise PublicationFailure(PublicationErrorCode.OPERATION_CONFLICT, publication_id)
+
</file context>
| if worktree.exists(): | ||
| raise CandidateFailure(CandidateErrorCode.WORKTREE_EXISTS, str(worktree)) | ||
| _git(root, "worktree", "add", "--detach", str(worktree), policy.initialization_commit) | ||
| source_commit = _accepted_source(root, policy) |
There was a problem hiding this comment.
P2: If the accepted ref advances during prepare, the second _accepted_source lookup can create a worktree from a commit different from the validated hypothesis source. Resolve the source once, validate that value, and use the same value for worktree creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/candidate_git.py, line 66:
<comment>If the accepted ref advances during `prepare`, the second `_accepted_source` lookup can create a worktree from a commit different from the validated hypothesis source. Resolve the source once, validate that value, and use the same value for worktree creation.</comment>
<file context>
@@ -60,11 +63,12 @@ def prepare(
if worktree.exists():
raise CandidateFailure(CandidateErrorCode.WORKTREE_EXISTS, str(worktree))
- _git(root, "worktree", "add", "--detach", str(worktree), policy.initialization_commit)
+ source_commit = _accepted_source(root, policy)
+ _git(root, "worktree", "add", "--detach", str(worktree), source_commit)
return CandidateWorkspace(
</file context>
| .current_accepted(root, policy.experiment_id, candidate_policy_digest(policy)) | ||
| .content_commit | ||
| ) | ||
| except PublicationFailure: |
There was a problem hiding this comment.
P2: When the evolution ledger is corrupt, busy, or unreadable, _accepted_source lets EvolutionLedgerFailure escape the candidate API. Catch EvolutionLedgerFailure here and map it to CandidateFailure, matching PublicationService's failure boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/candidate_git.py, line 167:
<comment>When the evolution ledger is corrupt, busy, or unreadable, `_accepted_source` lets `EvolutionLedgerFailure` escape the candidate API. Catch `EvolutionLedgerFailure` here and map it to `CandidateFailure`, matching `PublicationService`'s failure boundary.</comment>
<file context>
@@ -138,13 +143,31 @@ def _require_experiment(
+ .current_accepted(root, policy.experiment_id, candidate_policy_digest(policy))
+ .content_commit
+ )
+ except PublicationFailure:
+ raise CandidateFailure(CandidateErrorCode.STALE_COMMIT, policy.experiment_id) from None
+
</file context>
| _validate_operation(experiment_id, policy_digest, operation_id, publication_id) | ||
| intent, completion = self._operation(root, experiment_id, operation_id) | ||
| if completion is not None: | ||
| return self._completed(completion, publication_id, candidate_commit, candidate_tree) |
There was a problem hiding this comment.
P2: When the matching operation completed as a rollback, promote returns it as a successful publication. Require a ReleasePublished completion in this branch before calling _completed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/evolution/publication.py, line 287:
<comment>When the matching operation completed as a rollback, `promote` returns it as a successful publication. Require a `ReleasePublished` completion in this branch before calling `_completed`.</comment>
<file context>
@@ -0,0 +1,1059 @@
+ _validate_operation(experiment_id, policy_digest, operation_id, publication_id)
+ intent, completion = self._operation(root, experiment_id, operation_id)
+ if completion is not None:
+ return self._completed(completion, publication_id, candidate_commit, candidate_tree)
+ if intent is not None:
+ _check_intent(
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behavior
The accepted ref is derived as
refs/heads/ofw/<experiment-id>. Promotion validates the accepted gate, policy, candidate commit/tree, current CAS token, and ancestry before appending intent and advancing the ref withgit update-refusing the expected old commit. Recovery only reconciles durable intent with the observed ref; it never republishes implicitly.Rollback creates a new child commit with the historical target tree and current accepted commit as parent. It never rewinds, resets, force-updates, or rewrites history. Publication identity remains separate from Git tree/content identity.
Verification
uv run ruff check src tests plugins/openflywheel/scripts/mcp_server.pyuv run mypy src tests plugins/openflywheel/scripts/mcp_server.pyuv run pytest --cov=ofw --cov-report=term-missing --cov-fail-under=90 -q— 387 passed, 2 skipped; 93.75% total coverage; 92% publication-module coverageuvx --from radon radon cc -s -a src tests plugins/openflywheel/scripts/mcp_server.py— changed production functions at or below CC 5Scope
This branch is based on the latest PR4 controller head
cd5e2316fa2fcedae6f76afa7b286768d63dd945. No live adapters, generic release catalog, new dependency, or merge is included..omx/is local and untracked.Summary by cubic
Adds a typed accepted-publication service for prepared experiments and makes the accepted ref the durable source for subsequent candidates. Instead of relying on the initialization or controller commit, promotion now records intent and advances
refs/heads/ofw/<experiment-id>with CAS; rollback publishes a new child commit using a historical accepted tree, so Git history is never rewritten.New Features
PUBLISHand reports publication failures explicitly.Written for commit 5ca7088. Summary will update on new commits.