Implement post-submit setup derivation#88
Open
Abiorh001 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds setup-time post-submit checker derivation and compilation, persists provenance-bound policy outputs, introduces resumable Celery setup continuations with stale-worker handling, removes client-supplied post-submit policy inputs, and updates activation checks, documentation, planning state, and tests. ChangesPost-submit checker derivation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/app/workers/project_setup.py (1)
298-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated stale-recovery try/except across error branches.
The
ProjectServiceError(327-368) andException(387-420) branches each nest an identicaltry: update_project_setup_run_status(...) / except StaleProjectSetupContinuation: ...pattern to detect a concurrent successful compile before overwriting status. Extracting a small helper (e.g._record_terminal_status_or_recover(...)) would remove the duplication and reduce the risk of the two branches diverging over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/workers/project_setup.py` around lines 298 - 425, Extract the duplicated terminal-status update and stale-continuation recovery logic from the ProjectServiceError and generic Exception branches into a helper such as _record_terminal_status_or_recover. Have it call update_project_setup_run_status with the supplied status, error details, and continuation policy identifiers, return the post_submit_policy_compiled recovery result when applicable, and handle StaleProjectSetupContinuation consistently; update both branches to use the helper while preserving their existing fallback responses and logging.backend/alembic/versions/0014_post_submit_setup_continuation.py (1)
95-115: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMulti-column FKs use raw string names instead of
op.f(), inconsistent with line 89.Only the single-column
guide_idFK (Line 89) is wrapped inop.f(); the three composite hash-locking FKs (lines 96, 102, 109) use plain literal names. If a naming convention with anfkpattern is configured, these plain names may be silently overridden by the convention-derived name (ignoring the string you supplied), making the downgrade's matchingdrop constraint if exists fk_checker_policies_source_snapshot_hashetc. (lines 313-335) potentially incorrect too — again likely masked by cascadingDROP COLUMNcleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/alembic/versions/0014_post_submit_setup_continuation.py` around lines 95 - 115, Wrap the names of all three composite foreign keys in the upgrade—fk_checker_policies_source_snapshot_hash, fk_checker_policies_effective_policy_hash, and fk_checker_policies_pre_submit_checker_hash—with op.f(), matching the guide_id foreign key. Update the corresponding downgrade drop_constraint names to use op.f() as well, ensuring both upgrade and downgrade resolve the configured naming convention consistently.backend/app/modules/projects/setup_queue.py (1)
95-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating per-task Celery config sync.
Each task now repeats the same broker/result-backend/eager-propagation assignment block. As more setup continuation tasks get added, this will keep growing linearly and risks the two tasks drifting out of sync if one gets updated and the other doesn't.
♻️ Proposed refactor
def _sync_task_settings() -> None: """Sync mutable Celery task settings from the current test/runtime config.""" from app.workers.project_setup import ( run_post_submit_setup_continuation, run_pre_submit_setup_pipeline, ) settings = get_settings() - if settings.celery_broker_url is not None: - run_pre_submit_setup_pipeline.app.conf.broker_url = settings.celery_broker_url - run_post_submit_setup_continuation.app.conf.broker_url = settings.celery_broker_url - elif settings.celery_task_always_eager: - run_pre_submit_setup_pipeline.app.conf.broker_url = "memory://" - run_post_submit_setup_continuation.app.conf.broker_url = "memory://" - run_pre_submit_setup_pipeline.app.conf.result_backend = settings.celery_result_backend_url - run_pre_submit_setup_pipeline.app.conf.task_always_eager = settings.celery_task_always_eager - run_pre_submit_setup_pipeline.app.conf.task_eager_propagates = True - run_post_submit_setup_continuation.app.conf.result_backend = ( - settings.celery_result_backend_url - ) - run_post_submit_setup_continuation.app.conf.task_always_eager = ( - settings.celery_task_always_eager - ) - run_post_submit_setup_continuation.app.conf.task_eager_propagates = True + for task in (run_pre_submit_setup_pipeline, run_post_submit_setup_continuation): + if settings.celery_broker_url is not None: + task.app.conf.broker_url = settings.celery_broker_url + elif settings.celery_task_always_eager: + task.app.conf.broker_url = "memory://" + task.app.conf.result_backend = settings.celery_result_backend_url + task.app.conf.task_always_eager = settings.celery_task_always_eager + task.app.conf.task_eager_propagates = True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/setup_queue.py` around lines 95 - 118, Refactor _sync_task_settings to deduplicate the identical Celery configuration assignments for run_pre_submit_setup_pipeline and run_post_submit_setup_continuation. Define a shared collection of these task objects and apply broker_url, result_backend, task_always_eager, and task_eager_propagates uniformly in one loop, preserving the existing memory:// fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agent-loop/REVIEW_LOG.md:
- Line 23: Update the result entry in REVIEW_LOG.md so it does not claim
external review was addressed while that gate remains pending; either state that
external review is still pending or provide a verifiable link to the completed
external-review evidence before marking it complete.
In `@backend/app/modules/projects/models.py`:
- Around line 147-176: Add a backfill path in migration 0014_post_submit_setup
before enforcing NOT NULL on the new checker policy columns. Populate existing
checker_policies rows with valid derived or default values, then apply the
constraints; alternatively, explicitly enforce and document that the table must
be reset before upgrading. Update the migration logic and related model fields
consistently.
In `@backend/app/workers/project_setup.py`:
- Around line 273-297: Normalize every “post_submit_policy_compiled” return in
the setup flow, including the already_compiled branch and the success path in
run_post_submit_checker_policy_compilation, to include both
post_submit_checker_policy_id and idempotent. Obtain the existing policy ID for
already_compiled, use idempotent=True only for that branch and False for newly
compiled results, and ensure the race-recovery returns match the same shape.
---
Nitpick comments:
In `@backend/alembic/versions/0014_post_submit_setup_continuation.py`:
- Around line 95-115: Wrap the names of all three composite foreign keys in the
upgrade—fk_checker_policies_source_snapshot_hash,
fk_checker_policies_effective_policy_hash, and
fk_checker_policies_pre_submit_checker_hash—with op.f(), matching the guide_id
foreign key. Update the corresponding downgrade drop_constraint names to use
op.f() as well, ensuring both upgrade and downgrade resolve the configured
naming convention consistently.
In `@backend/app/modules/projects/setup_queue.py`:
- Around line 95-118: Refactor _sync_task_settings to deduplicate the identical
Celery configuration assignments for run_pre_submit_setup_pipeline and
run_post_submit_setup_continuation. Define a shared collection of these task
objects and apply broker_url, result_backend, task_always_eager, and
task_eager_propagates uniformly in one loop, preserving the existing memory://
fallback behavior.
In `@backend/app/workers/project_setup.py`:
- Around line 298-425: Extract the duplicated terminal-status update and
stale-continuation recovery logic from the ProjectServiceError and generic
Exception branches into a helper such as _record_terminal_status_or_recover.
Have it call update_project_setup_run_status with the supplied status, error
details, and continuation policy identifiers, return the
post_submit_policy_compiled recovery result when applicable, and handle
StaleProjectSetupContinuation consistently; update both branches to use the
helper while preserving their existing fallback responses and logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b1b81c9-c0ad-4ee8-9b69-bbb4895a3c1a
📒 Files selected for processing (27)
.agent-loop/LOOP_STATE.md.agent-loop/REVIEW_LOG.md.agent-loop/WORK_QUEUE.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/CHUNK_MAP.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/STATUS.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/chunks/WS-POL-002-02-post-submit-derivation-agent.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/chunks/WS-POL-002-03-post-submit-policy-approval-visibility.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/reviews/WS-POL-002-02-internal-review-evidence.md.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/reviews/WS-POL-002-02-pr-trust-bundle.mdREADME.mdbackend/alembic/versions/0014_post_submit_setup_continuation.pybackend/app/adapters/project_agents/openai_agent_sdk.pybackend/app/interfaces/project_agents.pybackend/app/modules/projects/models.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/workers/project_setup.pybackend/tests/test_agent_runtime.pybackend/tests/test_alembic.pybackend/tests/test_projects.pydocs/architecture_checker_framework.mddocs/architecture_data_model.mddocs/glossary.mddocs/operations_project_operating_manual.md
| - reuse/dedup | ||
| - test delta | ||
|
|
||
| Result: PASS after fixes; external review addressed; GitHub checks passed. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mark external review complete while it is still pending.
This entry says “external review addressed,” but the supplied PR context explicitly says external review remains pending. Update the result to reflect the pending gate, or link the actual external-review evidence before recording completion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agent-loop/REVIEW_LOG.md at line 23, Update the result entry in
REVIEW_LOG.md so it does not claim external review was addressed while that gate
remains pending; either state that external review is still pending or provide a
verifiable link to the completed external-review evidence before marking it
complete.
Comment on lines
+147
to
+176
| guide_id: Mapped[str] = mapped_column(ForeignKey("project_guides.id"), nullable=False, index=True) | ||
| guide_version: Mapped[str] = mapped_column(String(50), nullable=False) | ||
| source_snapshot_id: Mapped[str] = mapped_column( | ||
| ForeignKey("guide_source_snapshots.id"), | ||
| nullable=False, | ||
| index=True, | ||
| ) | ||
| source_snapshot_hash: Mapped[str] = mapped_column(String(71), nullable=False) | ||
| effective_policy_id: Mapped[str] = mapped_column(String(36), nullable=False, index=True) | ||
| effective_policy_hash: Mapped[str] = mapped_column(String(71), nullable=False, index=True) | ||
| pre_submit_checker_policy_id: Mapped[str] = mapped_column( | ||
| String(36), | ||
| nullable=False, | ||
| index=True, | ||
| ) | ||
| pre_submit_checker_bundle_hash: Mapped[str] = mapped_column( | ||
| String(71), | ||
| nullable=False, | ||
| index=True, | ||
| ) | ||
| required_checkers: Mapped[list[str]] = mapped_column(JSON, nullable=False, default=list) | ||
| warning_checkers: Mapped[list[str]] = mapped_column(JSON, nullable=False, default=list) | ||
| blocking_severities: Mapped[list[str]] = mapped_column(JSON, nullable=False, default=list) | ||
| policy_hash: Mapped[str | None] = mapped_column(String(71), index=True) | ||
| policy_body: Mapped[dict | None] = mapped_column(JSON) | ||
| lifecycle_status: Mapped[str] = mapped_column(String(30), nullable=False, default="compiled") | ||
| approved_by_role: Mapped[str | None] = mapped_column(String(50)) | ||
| approved_by_actor: Mapped[str | None] = mapped_column(String(100)) | ||
| approved_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) | ||
| created_by: Mapped[str] = mapped_column(String(100), nullable=False) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect migration 0014 for how new NOT NULL columns on checker_policies are added/backfilled.
fd -t f '0014_post_submit_setup' backend/alembic/versions --exec cat -n {}Repository: Flow-Research/workstream
Length of output: 14121
Add a backfill path before making these columns NOT NULL
0014_post_submit_setup currently aborts on any existing checker_policies rows instead of backfilling these fields. If this table is non-empty in upgrade environments, the migration will fail; add a migration path for existing data or make the reset requirement explicit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/modules/projects/models.py` around lines 147 - 176, Add a
backfill path in migration 0014_post_submit_setup before enforcing NOT NULL on
the new checker policy columns. Populate existing checker_policies rows with
valid derived or default values, then apply the constraints; alternatively,
explicitly enforce and document that the table must be reset before upgrading.
Update the migration logic and related model fields consistently.
Comment on lines
+273
to
+297
| if start_status == "already_compiled": | ||
| return {"status": "post_submit_policy_compiled", "idempotent": True} | ||
| policy, _, summary = await service.run_post_submit_checker_policy_derivation_agent( | ||
| actor, | ||
| project_id, | ||
| guide_id, | ||
| source_snapshot_id, | ||
| effective_policy_id, | ||
| pre_submit_checker_policy_id, | ||
| setup_run_id, | ||
| ) | ||
| await service.update_project_setup_run_status( | ||
| setup_run_id, | ||
| status="post_submit_policy_compiled", | ||
| current_step="post_submit_checker_policy_compilation", | ||
| output_post_submit_checker_policy_id=policy.id, | ||
| post_submit_derivation_summary=summary | ||
| | {"post_submit_checker_policy_id": policy.id}, | ||
| continuation_effective_policy_id=effective_policy_id, | ||
| continuation_pre_submit_checker_policy_id=pre_submit_checker_policy_id, | ||
| ) | ||
| return { | ||
| "status": "post_submit_policy_compiled", | ||
| "post_submit_checker_policy_id": policy.id, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent return-dict shape for post_submit_policy_compiled.
The "already_compiled" early return (Line 274) omits post_submit_checker_policy_id, while the success path (lines 294-297) omits idempotent, yet the race-recovery paths (345-352, 397-404) include both keys. Callers that index either key unconditionally on a differently-shaped branch will hit a KeyError.
🔧 Proposed fix
if start_status == "already_compiled":
- return {"status": "post_submit_policy_compiled", "idempotent": True}
+ return {
+ "status": "post_submit_policy_compiled",
+ "idempotent": True,
+ "post_submit_checker_policy_id": None,
+ }- return {
- "status": "post_submit_policy_compiled",
- "post_submit_checker_policy_id": policy.id,
- }
+ return {
+ "status": "post_submit_policy_compiled",
+ "idempotent": False,
+ "post_submit_checker_policy_id": policy.id,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if start_status == "already_compiled": | |
| return {"status": "post_submit_policy_compiled", "idempotent": True} | |
| policy, _, summary = await service.run_post_submit_checker_policy_derivation_agent( | |
| actor, | |
| project_id, | |
| guide_id, | |
| source_snapshot_id, | |
| effective_policy_id, | |
| pre_submit_checker_policy_id, | |
| setup_run_id, | |
| ) | |
| await service.update_project_setup_run_status( | |
| setup_run_id, | |
| status="post_submit_policy_compiled", | |
| current_step="post_submit_checker_policy_compilation", | |
| output_post_submit_checker_policy_id=policy.id, | |
| post_submit_derivation_summary=summary | |
| | {"post_submit_checker_policy_id": policy.id}, | |
| continuation_effective_policy_id=effective_policy_id, | |
| continuation_pre_submit_checker_policy_id=pre_submit_checker_policy_id, | |
| ) | |
| return { | |
| "status": "post_submit_policy_compiled", | |
| "post_submit_checker_policy_id": policy.id, | |
| } | |
| if start_status == "already_compiled": | |
| return { | |
| "status": "post_submit_policy_compiled", | |
| "idempotent": True, | |
| "post_submit_checker_policy_id": None, | |
| } | |
| policy, _, summary = await service.run_post_submit_checker_policy_derivation_agent( | |
| actor, | |
| project_id, | |
| guide_id, | |
| source_snapshot_id, | |
| effective_policy_id, | |
| pre_submit_checker_policy_id, | |
| setup_run_id, | |
| ) | |
| await service.update_project_setup_run_status( | |
| setup_run_id, | |
| status="post_submit_policy_compiled", | |
| current_step="post_submit_checker_policy_compilation", | |
| output_post_submit_checker_policy_id=policy.id, | |
| post_submit_derivation_summary=summary | |
| | {"post_submit_checker_policy_id": policy.id}, | |
| continuation_effective_policy_id=effective_policy_id, | |
| continuation_pre_submit_checker_policy_id=pre_submit_checker_policy_id, | |
| ) | |
| return { | |
| "status": "post_submit_policy_compiled", | |
| "idempotent": False, | |
| "post_submit_checker_policy_id": policy.id, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/workers/project_setup.py` around lines 273 - 297, Normalize every
“post_submit_policy_compiled” return in the setup flow, including the
already_compiled branch and the success path in
run_post_submit_checker_policy_compilation, to include both
post_submit_checker_policy_id and idempotent. Obtain the existing policy ID for
already_compiled, use idempotent=True only for that branch and False for newly
compiled results, and ensure the race-recovery returns match the same shape.
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.
PR Trust Bundle: WS-POL-002-02
Chunk
WS-POL-002-02- Post-Submit Derivation Agent And Resumable Setup IntegrationGoal
Add setup-time post-submit checker derivation and compilation after
submission artifact policy approval and pre-submit checker compilation, without
introducing runtime agent judgment or per-task checker generation.
Human-Approved Intent
Post-submit checkers must follow the same shape as pre-submit:
The agent derives a constrained specification during setup. Workstream compiles
and owns the deterministic checker policy. The agent never judges worker
submissions at runtime.
What Changed
0014_post_submit_setupfor post-submit setup provenance.Design Chosen
The existing project setup queue and worker were extended. No disconnected
post-submit-only queue was added.
The setup pipeline now continues like this:
The post-submit policy remains project-scoped. Tasks will later lock the
project policy reference; they do not derive or compile their own checker
bundle.
Alternatives Rejected
Scope Control
No task runtime module changes, frontend/demo work, payment, reputation,
blockchain, marketplace, or settlement work were included.
The route change is docstring-only to remove stale OpenAPI wording that implied
manual post-submit policy updates.
Product Behavior
Setup-authorized admins/project managers can approve the submission artifact
policy. That approval creates the effective project policy and compiled
pre-submit checker bundle, then resumes setup into post-submit derivation.
Unsupported post-submit checker requirements block setup with bounded
operator-visible details. Workers and reviewers do not receive internal setup
defects as product review decisions.
Guide activation still requires setup approval of the compiled post-submit
policy. This chunk creates the compiled generated output;
WS-POL-002-03implements the server-owned visibility and approval surface.
Acceptance Criteria Proof
PostSubmitCheckerPolicy.Tests/Checks Run
Result summary:
Reviewer Results
Internal review evidence:
.agent-loop/initiatives/WS-POL-002-post-submit-checker-foundation/reviews/WS-POL-002-02-internal-review-evidence.mdReviewed code SHA:
9179f9dced4b5b58c298cb1f93149c26d6d2b6c3Reviewed at:
2026-07-10T05:27:44ZExternal Review
External review is pending for this PR. CodeRabbit and GitHub Actions must run
after the branch is pushed, and any actionable comments must be handled in a
separate external review response file.
Remaining Risks
and setup-run compiled-status write. Retry/operator repair can recover; later
hardening can make first compiled policy win by provenance.
exception semantics. This is accepted for this chunk but should be extracted
if setup policy grows further.
Human Review Focus
Human Merge Ownership
Only the user can approve and merge this PR. Codex must not merge it without
explicit user approval for that specific PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation