Prove Terminal Benchmark setup-agent drill#67
Merged
Conversation
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/architecture_data_model.md (1)
126-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winProjectGuide field list is missing
statusandupdated_at.
backend/app/modules/projects/models.pystill definesstatusandupdated_atcolumns onProjectGuide, but the rewritten field list here only listsid,project_id,version,content_markdown,change_summary,approved_by,effective_at,created_by,created_at,superseded_at.As per coding guidelines, `**/*.md` files should "Prefer evidence-backed docs over vague product claims" — the field list should stay accurate to the actual model.📝 Proposed fix
- `id` - `project_id` - `version` +- `status` - `content_markdown` - `change_summary` - `approved_by` - `effective_at` - `created_by` - `created_at` +- `updated_at` - `superseded_at`🤖 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 `@docs/architecture_data_model.md` around lines 126 - 146, The ProjectGuide field list is out of sync with the actual model because it omits `status` and `updated_at`. Update the field inventory in this architecture doc to match `ProjectGuide` as defined in `backend/app/modules/projects/models.py`, keeping the documented persisted/server-written fields accurate. Use the `ProjectGuide` model and its field list as the source of truth, and make sure the markdown reflects all columns currently defined rather than a partial subset.Source: Coding guidelines
🧹 Nitpick comments (2)
backend/app/modules/tasks/service.py (1)
891-920: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim unused
field_valuesentries.Only
title,description, andacceptance_criteriaare checked againstrequired_fields, butfield_valuesstill carries 8 other keys (task_type,difficulty,skill_tags,estimated_time_minutes,rejection_criteria,required_files,required_evidence,deadline_at) left over from the old guide-driven validator. This dead code could mislead future maintainers into thinking these fields are enforced here.♻️ Proposed cleanup
required_fields = {"title", "description", "acceptance_criteria"} field_values = { "title": task.title, "description": task.description, - "task_type": task.task_type, - "difficulty": task.difficulty, - "skill_tags": task.skill_tags, - "estimated_time_minutes": task.estimated_time_minutes, "acceptance_criteria": task.acceptance_criteria, - "rejection_criteria": task.rejection_criteria, - "required_files": task.required_files, - "required_evidence": task.required_evidence, - "deadline_at": task.deadline_at, }🤖 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/tasks/service.py` around lines 891 - 920, The `_validate_task_contract_fields` method still builds a `field_values` map with several entries that are never checked by `required_fields`, which is misleading dead code. Remove the unused keys from `field_values` so it only includes the fields actually validated here (`title`, `description`, `acceptance_criteria`), and keep the missing-field check and `TaskValidationError` behavior unchanged.examples/terminal_benchmark/terminal_benchmark_api_e2e.py (1)
117-134: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
reviewer_root()walks all ancestor directories up to filesystem root.If
WORKSTREAM_TERMIUS_REVIEWER_ROOTis unset, this searchesfixture_root_pathand every parent up to/forPROJECT_GUIDE.md/REVIEWER_PROGRAM.md, silently accepting the first match found anywhere in the ancestry. An unrelated pair of similarly-named files higher up the tree would be picked up without any error, feeding wrong guide content into the "real fixture" drill.♻️ Suggested bound on traversal
- candidates.extend([fixture_root_path, *fixture_root_path.parents]) + candidates.extend([fixture_root_path, fixture_root_path.parent])🤖 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 `@examples/terminal_benchmark/terminal_benchmark_api_e2e.py` around lines 117 - 134, The reviewer_root() helper currently searches fixture_root_path and every ancestor up to the filesystem root, which can accidentally pick up unrelated PROJECT_GUIDE.md and REVIEWER_PROGRAM.md files. Update the candidate traversal in reviewer_root() so it is bounded to the intended reviewer root scope (for example by limiting how far parents are checked or by requiring an explicit configured root when needed), while still using the existing REVIEWER_ROOT_ENV_VAR and the PROJECT_GUIDE.md/REVIEWER_PROGRAM.md presence check to locate the correct root.
🤖 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 `@backend/alembic/versions/0010_remove_legacy_project_guide_fields.py`:
- Around line 37-55: The guard in _legacy_guide_snapshots_exist() is too broad
because it blocks on any guide_source_snapshots row, including current
guide-source-manifest/v1 data. Update the check in this helper used by upgrade()
to only detect legacy pre-0010 snapshot records by filtering on the old
shape/version fields, or otherwise make the migration explicitly safe only for
empty/dev databases. Keep the RuntimeError in upgrade() tied to that narrower
legacy-only condition.
---
Outside diff comments:
In `@docs/architecture_data_model.md`:
- Around line 126-146: The ProjectGuide field list is out of sync with the
actual model because it omits `status` and `updated_at`. Update the field
inventory in this architecture doc to match `ProjectGuide` as defined in
`backend/app/modules/projects/models.py`, keeping the documented
persisted/server-written fields accurate. Use the `ProjectGuide` model and its
field list as the source of truth, and make sure the markdown reflects all
columns currently defined rather than a partial subset.
---
Nitpick comments:
In `@backend/app/modules/tasks/service.py`:
- Around line 891-920: The `_validate_task_contract_fields` method still builds
a `field_values` map with several entries that are never checked by
`required_fields`, which is misleading dead code. Remove the unused keys from
`field_values` so it only includes the fields actually validated here (`title`,
`description`, `acceptance_criteria`), and keep the missing-field check and
`TaskValidationError` behavior unchanged.
In `@examples/terminal_benchmark/terminal_benchmark_api_e2e.py`:
- Around line 117-134: The reviewer_root() helper currently searches
fixture_root_path and every ancestor up to the filesystem root, which can
accidentally pick up unrelated PROJECT_GUIDE.md and REVIEWER_PROGRAM.md files.
Update the candidate traversal in reviewer_root() so it is bounded to the
intended reviewer root scope (for example by limiting how far parents are
checked or by requiring an explicit configured root when needed), while still
using the existing REVIEWER_ROOT_ENV_VAR and the
PROJECT_GUIDE.md/REVIEWER_PROGRAM.md presence check to locate the correct root.
🪄 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: 79da963d-b8ae-4b5e-9281-7b87495face0
📒 Files selected for processing (36)
.agent-loop/LOOP_STATE.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/CHUNK_MAP.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/DISCOVERY.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/INTENT.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/PLAN.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/RISKS.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/STATUS.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/chunks/WS-POL-001-06-terminal-benchmark-real-fixture-drill.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-06-internal-review-evidence.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-06-pr-trust-bundle.mdREADME.mdbackend/alembic/versions/0010_remove_legacy_project_guide_fields.pybackend/app/adapters/project_agents/local_fixture.pybackend/app/modules/projects/models.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/tasks/service.pybackend/tests/test_alembic.pybackend/tests/test_checkers.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/architecture_data_model.mddocs/architecture_lockdown.mddocs/architecture_system_architecture.mddocs/operations_operator_workflow.mddocs/operations_project_operating_manual.mddocs/operations_queue_policy.mddocs/operations_reviewer_workflow.mddocs/product_first_user_flows.mddocs/roadmap_implementation_backlog.mddocs/spec_chunk_3_project_guide_foundation.mddocs/spec_chunk_4_task_queue_assignment.mddocs/template_project_guide.mddocs/template_task.mdexamples/terminal_benchmark/README.mdexamples/terminal_benchmark/terminal_benchmark_api_e2e.py
💤 Files with no reviewable changes (2)
- backend/tests/test_checkers.py
- backend/app/modules/projects/service.py
✅ Files skipped from review due to trivial changes (15)
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/RISKS.md
- docs/spec_chunk_4_task_queue_assignment.md
- docs/operations_reviewer_workflow.md
- README.md
- docs/roadmap_implementation_backlog.md
- docs/operations_operator_workflow.md
- docs/operations_queue_policy.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/PLAN.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/INTENT.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/DISCOVERY.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-06-internal-review-evidence.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/STATUS.md
- docs/architecture_system_architecture.md
- examples/terminal_benchmark/README.md
- .agent-loop/LOOP_STATE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/chunks/WS-POL-001-06-terminal-benchmark-real-fixture-drill.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/CHUNK_MAP.md
Comment on lines
+37
to
+55
| def _legacy_guide_snapshots_exist() -> bool: | ||
| """Return whether old snapshot provenance must be rebuilt before cleanup.""" | ||
| bind = op.get_bind() | ||
| count = bind.scalar(sa.text("select count(*) from guide_source_snapshots")) | ||
| return bool(count) | ||
|
|
||
|
|
||
| def upgrade() -> None: | ||
| """Remove stale guide fields and project-owned payment duplicates.""" | ||
| if _legacy_guide_snapshots_exist(): | ||
| raise RuntimeError( | ||
| "cannot drop legacy project guide fields while guide source snapshots exist; " | ||
| "recreate setup data with fresh guide source snapshots under the current " | ||
| "content_markdown-only guide contract" | ||
| ) | ||
| op.drop_column("projects", "base_amount") | ||
| op.drop_column("projects", "currency") | ||
| for column_name in LEGACY_COLUMNS: | ||
| op.drop_column("project_guides", column_name) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files backend/alembic/versions/0010_remove_legacy_project_guide_fields.py \
backend/alembic/versions \
backend | sed -n '1,120p'
printf '\n== Migration file ==\n'
cat -n backend/alembic/versions/0010_remove_legacy_project_guide_fields.py
printf '\n== Search guide_source_snapshots references ==\n'
rg -n "guide_source_snapshots|source_snapshot_payload|manifest_schema_version|manifest_json|legacy_guide_snapshots_exist|content_markdown-only" backend -g '*.py'
printf '\n== Tests around migration ==\n'
rg -n "0010_remove_legacy_project_guide_fields|legacy_guide_snapshots_exist|guide_source_snapshots" backend -g 'test*.py' -g '*test*.py'
printf '\n== Snapshot table / model definitions ==\n'
rg -n "guide_source_snapshots|GuideSourceSnapshot|source_snapshot_payload" backend -g '*.py'
printf '\n== Relevant schema migration history ==\n'
rg -n "create_table\\(|guide_source_snapshots" backend/alembic/versions -g '*.py'Repository: Flow-Research/workstream
Length of output: 37218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '520,580p' backend/app/modules/projects/service.py
printf '\n==\n'
sed -n '1570,1605p' backend/app/modules/projects/service.py
printf '\n==\n'
sed -n '280,330p' backend/tests/test_alembic.pyRepository: Flow-Research/workstream
Length of output: 5926
Guard should target legacy snapshots, not the whole table
_legacy_guide_snapshots_exist() counts every guide_source_snapshots row, so this migration will abort in any database that already has current guide-source-manifest/v1 snapshots, not just legacy pre-0010 data. Scope the check to the old shape/version, or document that this migration is only safe on empty/dev databases.
🤖 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/0010_remove_legacy_project_guide_fields.py` around
lines 37 - 55, The guard in _legacy_guide_snapshots_exist() is too broad because
it blocks on any guide_source_snapshots row, including current
guide-source-manifest/v1 data. Update the check in this helper used by upgrade()
to only detect legacy pre-0010 snapshot records by filtering on the old
shape/version fields, or otherwise make the migration explicitly safe only for
empty/dev databases. Keep the RuntimeError in upgrade() tied to that narrower
legacy-only condition.
abiorh-claw
approved these changes
Jul 5, 2026
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
2 participants
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-001-06
Chunk
WS-POL-001-06- Terminal Benchmark Real Fixture DrillGoal
Use a real Terminal Benchmark reviewer fixture as an external-project proof for
the current Workstream setup-agent, project policy-bundle, task locked-context,
pre-submit, post-submit checker, and revision resubmission lifecycle.
This pass also cleans up stale construction-state product contracts before
continuing pre-submit checker work.
Human-Approved Intent
The user explicitly pushed back that:
PaymentPolicy;ProjectGuideis human-facing guide material, not a machine-readableartifact checklist;
evidence_policyand guide checklist fields should be removed, notpreserved through compatibility aliases;
product runtime.
What Changed
0010_remove_legacy_project_guide_fields.py.base_amountandcurrencycolumns from the currentproject shape.
shape, including
required_task_fields,required_submission_fields,reviewer_rubric,forbidden_actions,evidence_policy, and relatedchecklist fields.
approved_byandeffective_atas server-written guide activationprovenance and exposes them on read responses.
approved_byandeffective_atforbidden in create/update requestbodies.
0010fail closed when existingguide_source_snapshotsare present,forcing stale setup data to be rebuilt under the current guide-source
contract.
guide-side required fields.
task-visible payout fields are locked snapshots from
PaymentPolicy.adapter and real Termius project guide, reviewer program, task TOML, and
review packet material.
fixture adapter.
contracts: shell project creation, payment terms only under
PaymentPolicy,human-facing guide content plus policy records, and no task
required_files/required_evidenceintake-policy inputs.guard, aligning the
ProjectGuidefield list with the ORM model, removingdead task-validation map entries, and bounding Terminal Benchmark
reviewer-root discovery.
cleanup, stale snapshot fail-closed behavior, task/checker behavior, and the
example contract.
Design Chosen
The current v0.1 contract is:
There is no compatibility alias for
ProjectGuide.evidence_policyor guide-siderequired fields. Existing pre-production setup data with guide-source snapshots
must be rebuilt instead of silently carrying stale provenance through a
destructive cleanup.
Alternatives Rejected
base_amount/currency: rejected because payment belongs toPaymentPolicy.those fields were construction-state shortcuts and would keep the wrong
mental model alive.
may have been calculated from now-deleted guide fields.
example must not become product runtime.
local_fixture: rejectedbecause this proof is meant to exercise the setup-agent route.
Scope
Runtime/backend:
backend/alembic/versions/0010_remove_legacy_project_guide_fields.pybackend/app/adapters/project_agents/local_fixture.pybackend/app/modules/projects/models.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/tasks/service.pyTests:
backend/tests/test_alembic.pybackend/tests/test_checkers.pybackend/tests/test_projects.pybackend/tests/test_tasks.pybackend/scripts/week1_api_e2e.pyDocs/examples/process:
.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/**README.mddocs/architecture_data_model.mddocs/architecture_lockdown.mddocs/architecture_system_architecture.mddocs/operations_operator_workflow.mddocs/operations_project_operating_manual.mddocs/operations_queue_policy.mddocs/operations_reviewer_workflow.mddocs/product_first_user_flows.mddocs/roadmap_implementation_backlog.mddocs/spec_chunk_3_project_guide_foundation.mddocs/spec_chunk_4_task_queue_assignment.mddocs/template_project_guide.mddocs/template_task.mdexamples/terminal_benchmark/**Product Behavior
base_amountandcurrency.server-written activation provenance fields.
approved_byandeffective_at.pre_submission_checker_failed;it does not create a submission and does not become
needs_revision.needs_revision.review_pending.Verification
Passed locally:
Results:
tests/test_alembic.py: 6 passedtests/test_checkers.py: 47 passedtests/test_tasks.py: 60 passedtests/test_projects.py: 188 passedscripts/week1_api_e2e.py: passed against local Postgres on 2026-07-05tests/test_alembic.py: 6 passedInternal Review
Evidence:
.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-06-internal-review-evidence.mdReviewed implementation SHA:
96792961c7cb74f31150df803c533fe4c6432636Reviewer summary:
External Review
External GitHub Actions and CodeRabbit must rerun after this evidence/status
commit is pushed. Do not treat the previous PR check state as current for the
new commits.
Remaining Risks
runtime.
configuration.
guide_source_snapshots; any such local pre-production data must be rebuiltthrough the current policy-bundle path.
Human Review Focus
PaymentPolicy.ProjectGuideis now human-facing guide material only.0010intentionally fails closed for old snapshot provenance.Human Merge Ownership
Only the user can approve and merge the PR. The agent must not merge without
explicit user approval for that specific PR.