No-QA generation runs and TEI-P5 retrieval (4.3.2) - #141
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
SummaryImplements roadmap task 4.3.2: no-QA source-to-script REST workflows with durable, idempotent generation runs and TEI-P5 retrieval. Highlights
Documentation
TestingAdded unit, persistence, API, launcher and Vidai Mock behavioural coverage, including seven end-to-end scenarios. Validation reports 1,076 passing tests, one skipped test and seven passing Vidai Mock scenarios. WalkthroughAdds durable generation-run and event persistence, optimistic TEI revisioning, validated no-QA LLM draft generation, asynchronous launcher orchestration, cost recording, REST retrieval, and acceptance-test coverage. ChangesNo-QA generation slice
Sequence Diagram(s)sequenceDiagram
participant Client
participant GenerationRunsResource
participant InProcessGenerationRunLauncher
participant SqlAlchemyGenerationRunStore
participant LLMDraftScriptGenerator
participant SqlAlchemyEpisodeRepository
Client->>GenerationRunsResource: POST draft_without_qa request
GenerationRunsResource->>SqlAlchemyGenerationRunStore: create pending run
GenerationRunsResource->>InProcessGenerationRunLauncher: launch run
InProcessGenerationRunLauncher->>SqlAlchemyGenerationRunStore: claim pending run
InProcessGenerationRunLauncher->>LLMDraftScriptGenerator: generate draft
InProcessGenerationRunLauncher->>SqlAlchemyEpisodeRepository: persist TEI with revision precondition
InProcessGenerationRunLauncher->>SqlAlchemyGenerationRunStore: append lifecycle events and update status
Client->>GenerationRunsResource: poll run and fetch TEI
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
c5422af to
0d648a9
Compare
lodyai
Bot
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
leynos
changed the title
lodyai
Bot
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Excess Number of Function Argumentstests/test_generation_run_port_contract.py: NoopGenerationRunPort.update_run_status has 6 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. Overall Code Complexityepisodic/canonical/adapters/generation_runs.py: What lead to degradation?This module has a mean cyclomatic complexity of 4.10 across 10 functions. The mean complexity threshold is 4 Why does this problem occur?Overall Code Complexity is measured by the mean cyclomatic complexity across all functions in the file. The lower the number, the better. How to fix it?You address the overall cyclomatic complexity by a) modularizing the code, and b) abstract away the complexity. Let's look at some examples: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
34d6f58 to
a3d64bb
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. episodic/canonical/adapters/generation_runs.py Comment on file from episodic.canonical.domain import (
Checkpoint,
CheckpointResponse,❌ New issue: Overall Code Complexity |
This comment was marked as resolved.
This comment was marked as resolved.
Draft the execution plan for roadmap task 4.3.2, the second half of the
source-to-script REST vertical slice (ADR 009). The plan delivers
`draft_without_qa` generation-run creation, REST polling of the run and
its event log, persistence of the generated script into the canonical
episode TEI with QA marked skipped, and `GET /v1/episodes/{id}/tei` as a
JSON envelope or `application/tei+xml` download.
Key confirmed decisions: in-process async execution behind a launcher
port with durable run/event records (Celery deferred per ADR 007);
episode materialised from the ingestion job; a minimal real
DraftScriptGenerator behind a port (full QA-bypass graph left to 4.4.1).
The plan was stress-tested by a Logisphere community-of-experts design
review and revised accordingly: detached-task unit-of-work isolation,
durable stuck-run hooks and conditional state transitions, idempotency
replay carrying Location/Retry-After, deterministic TEI ids and clock
injection for stable snapshots, explicit cost-recorder wiring, and a
must-run-once Vidai Mock acceptance gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mark the 4.3.2 ExecPlan as in progress, record the branch and PR metadata findings, and update the concrete worktree path. Include Markdown formatter output required by the documentation gate.
Capture the source-to-script REST slice as strict-xfail BDD scenarios. Record the red scaffold evidence and deterministic M0 gate results in the living ExecPlan.
Mark the red behavioural scaffold milestone complete after deterministic gates and CodeRabbit review completed with zero findings.
Introduce `QualityMode` and `QaStatus` for the draft-without-QA vertical slice, and validate that generation runs record skipped QA status with a non-empty bypass rationale. Update generation-run fixtures, snapshots, and architecture-prefix configuration so the new domain module is covered by the existing gates. Record the M1 red, green, and deterministic-gate evidence in the ExecPlan.
Mark the no-QA generation-run metadata milestone complete in the ExecPlan and record the CodeRabbit rate-limit retry plus zero-finding review result.
Introduce SQLAlchemy generation-run and event-log tables plus the matching repository adapter and unit-of-work wiring. The adapter preserves idempotent run creation, uses an explicit guarded claim operation for pending-to-running execution, and serializes event sequence allocation by locking the parent run row. Split the in-memory checkpoint methods into a focused mixin so the existing test adapter stays within the project file-size limit while the run/event surface grows.
Mark the durable generation-run persistence milestone complete after the full deterministic gates and CodeRabbit review completed without findings.
Add optimistic episode TEI update metadata, storage mapping, and SQL migration support for no-QA generation persistence. Split the SQLAlchemy episode repository and TEI update tests so the changed modules stay focused while preserving existing episode storage behaviour.
Mark the episode TEI revisioning milestone complete after deterministic gates and a zero-finding CodeRabbit review.
Add a single-pass draft script generator that emits validated TEI-P5 through tei_rapporteur and records deterministic content hashes. Add application persistence services that materialise episodes from intake sources, project provenance source documents, and write generated no-QA TEI with optimistic episode revisioning.
Resolve series-level host and guest profile revisions when claiming a run and project them into the draft request. Exercise real presenter document, revision, and binding setup in the end-to-end behavioural slice.
Record the launcher, persistence, recovery, and content-negotiation decisions in ADR 016. Update system, user, maintainer, and repository guidance to match the completed implementation and close roadmap item 4.3.2.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move scenario teardown into the support context so asynchronous resources and the Vidai Mock process retain one lifecycle owner while the step module stays within the repository's 400-line policy.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Load upload-backed source text before draft generation, and retain resolved presenter context in launcher requests. Serialize episode materialization through the ingestion job and persist its target episode so retries converge. Lock generation-run rows before status mutation so competing terminal transitions preserve terminal immutability.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove superseded progress and retrospective statements from the ExecPlan. Record the completed correctness review, final gate totals, and confirmed roadmap completion as the current implementation state.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace the stale in-progress header and intermediate audit language with the completed implementation state. Preserve red-state transcripts as historical evidence and record the final behavioural and correctness-review results.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace the second-person success wording in the 4.3.2 ExecPlan with impersonal language, add a brief review-follow-up note for the verified live fixes and stale findings, and correct the generation-run route placeholder in the user guide. The route now matches the implemented endpoint.
Document the in-memory checkpoint mixin and its public methods with NumPy-style purpose, usage, return, and error details. Also add the missing `EpisodeRecord` attribute entries for the TEI revision and generation-quality fields.
Rephrase the review-follow-up entry to stay concise and impersonal while preserving the live-fix/stale-findings status.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Wire the configured LLM launcher, enforce generation eligibility and TEI invariants, preserve cancellation semantics, and strengthen adapter tests.
Document the current review-follow-up state at commit `42757a3`, including the valid fixes, stale findings skipped, deliberate non-changes, and the 2026-07-23 gate evidence.
Append the final CodeRabbit review record to the latest review-follow-up entry, noting the f8609b4 run on 2026-07-23 and its terminal review_completed result with zero findings.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract cohesive validation and test helpers while preserving runtime and domain behaviour, and add diagnostic context to launcher assertions.
There was a problem hiding this comment.
Gates Failed
(1 file with Overall Code Complexity)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| generation_runs.py | 1 advisory rule | no change | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
episodic/canonical/storage/repositories.py (1)
171-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale
SqlAlchemyIngestionJobRepositorycopy. Keep the implementation inepisodic/canonical/storage/ingestion_job_repositories.py;episodic/canonical/storage/uow.pyandepisodic/canonical/storage/__init__.pyalready import that module, so the duplicate inepisodic/canonical/storage/repositories.py#L171-L217only creates drift.🤖 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 `@episodic/canonical/storage/repositories.py` around lines 171 - 217, Remove the duplicate SqlAlchemyIngestionJobRepository implementation from episodic/canonical/storage/repositories.py lines 171-217. Retain the canonical implementation in episodic/canonical/storage/ingestion_job_repositories.py lines 26-65; no direct change is needed there, and existing imports in uow.py and __init__.py should continue referencing it.
🤖 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 `@docs/contents.md`:
- Line 100: Wrap the ADR 016 links to remain within 80 columns while preserving
their titles and targets. Update docs/contents.md lines 100-100 and
docs/developers-guide.md lines 25-25, using reference-style links or another
equivalent wrapped format at both sites.
In `@docs/execplans/4-3-2-no-qa-generation-runs-and-tei-p5-retrieval.md`:
- Around line 1010-1024: Update the embedded feature narrative to use impersonal
wording instead of “As an integration client,” “I want,” and “So that I can.”
Also replace “my rationale” in the scenario outcome with neutral wording while
preserving the intended behavior and meaning.
In `@episodic/api/resources/generation_runs.py`:
- Line 3: Remove the `from __future__ import annotations` statement from the
module so annotations for `UowFactory`, `JsonPayload`, `GenerationRunLauncher`,
and `cabc.Callable` are evaluated normally under Python 3.14.
In `@episodic/api/runtime.py`:
- Around line 79-126: Replace the generic RuntimeError exceptions raised by
_required_setting and _llm_settings with ValueError or a dedicated
configuration-specific exception. Update the handler in _load_runtime_config to
catch that exact configuration error type while preserving the existing warning
and re-raise behavior; do not catch unrelated runtime faults.
In `@episodic/api/serializers.py`:
- Around line 247-259: Update serialize_tei_envelope to import and reference
QualityMode.DRAFT_WITHOUT_QA.value for quality_mode instead of hardcoding
"draft_without_qa". Keep the remaining envelope fields unchanged and align this
serializer with serialize_generation_run.
In `@episodic/canonical/adapters/generation_runs.py`:
- Around line 62-70: Consolidate mutation-failure logging by introducing a
frozen, slotted _RunMutationLogSpec near _require_mutable_run and updating that
method to accept the spec, log operation-specific missing and terminal events
with shared and extra fields, then raise the existing exceptions. Replace the
duplicated try/except handling in update_run_status, claim_run_for_execution,
and append_event with appropriate specs while preserving their current event
names and fields.
In `@episodic/canonical/domain.py`:
- Around line 348-352: Update _require_value so a None provenance value raises
ValueError instead of TypeError, preserving the existing message and validation
flow to remain consistent with the sibling domain-validation helpers.
- Around line 383-386: Update the EpisodeTeiUpdate dataclass decorator to
include slots=True, matching the existing structured-data dataclasses
GenerationRunStatusUpdate and _CheckpointTransitionSpec.
In `@episodic/canonical/generation_persistence.py`:
- Around line 84-121: Review materialise_episode_from_ingestion and
_get_ingestion_job_for_update to narrow the ingestion-job row lock: claim and
persist the target episode identity under the lock, then release it before the
per-source insertion work and remaining materialisation steps. Preserve existing
READY_FOR_GENERATION validation and duplicate-episode behavior while ensuring
concurrent retries cannot claim different episode IDs.
- Around line 46-52: Split the direct DraftScriptPersistenceError raises in the
generation persistence flow into concrete IngestionJobNotReadyError,
MissingAttachedSourcesError, and UploadNotFoundError subclasses, keeping them
under the shared base and naming them with the Error suffix. Give each exception
a structured attribute for its relevant identifier, update the “not ready,” “no
attached sources,” and “upload not found” call sites to raise the appropriate
subtype, and provide a dedicated subtype for the content-hash mismatch as well.
- Around line 295-302: Update _source_content_hash to use structural pattern
matching when extracting and validating source.metadata["content_hash"] instead
of the isinstance() guard, while preserving the existing non-empty string
behavior and fallback order.
- Around line 32-33: Update the Clock and UuidFactory type aliases to use the
Python 3.14 PEP 695 type statement instead of plain assignments, preserving
their existing callable signatures.
In `@episodic/canonical/storage/entity_mappers.py`:
- Around line 140-143: Update the entity mapping around the persisted
tei_content_hash assignment to always derive the hash by calling
sha256_text(episode.tei_xml), removing the fallback to episode.tei_content_hash.
Add a test using a deliberately stale supplied hash and verify the persisted
value matches the current TEI XML.
In `@episodic/canonical/storage/entity_models.py`:
- Around line 145-153: Update the episode entity model around qa_status and
last_generation_run_id to persist quality_mode and the skip-QA rationale as
nullable mapped fields, add the corresponding database migration, and expose
both fields through the episode mapper/API; alternatively, revise the documented
and API contract to resolve this provenance exclusively from the linked
generation run.
In `@episodic/canonical/storage/generation_runs.py`:
- Around line 239-276: Update claim_run_for_execution to call _log_event for
both outcomes: after successfully transitioning a pending run to running, and
when the conditional update loses the race and returns None. Include the run
identifier and appropriate claim outcome details, while preserving the existing
terminal and not-found exception behavior.
- Line 22: Update claim_run_for_execution to emit a _log_event after
successfully transitioning the run from pending to running. Include the claim
transition details consistently with the existing create_run, update_run_status,
and append_event lifecycle logs, using the imported _log_event helper.
In `@episodic/generation/launcher_support.py`:
- Around line 303-313: Add explicit entries to _FAILURE_CATEGORIES for
EpisodeRevisionConflictError and EpisodeNotFoundError, mapping them to a
dedicated episode-persistence conflict category and the appropriate
retry/terminal flag. Ensure persist_draft_script failures from
uow.episodes.update are classified as this first-class conflict rather than the
generic unexpected category.
In `@episodic/generation/launcher.py`:
- Line 365: Update the exception handler around RunAlreadyTerminal and
RunNotFound to use the parenthesized multi-exception form, preserving the
existing handler behavior.
In `@tests/canonical_storage/test_episode_tei_updates.py`:
- Around line 136-153: Type the session_factory parameter in all three affected
tests as the quoted async_sessionmaker[AsyncSession] type already supported by
the TYPE_CHECKING imports, and remove the redundant typ.cast assignments. Keep
the existing factory usage unchanged so type checking validates fixture
compatibility at each test call site.
In `@tests/canonical_storage/test_generation_runs.py`:
- Around line 258-292: Add a concurrent SQL-adapter test alongside
test_generation_run_store_claims_pending_run_once that creates one pending run,
opens separate sessions, and invokes claim_run_for_execution for the same run_id
concurrently via asyncio.gather(). Assert exactly one claim returns a result and
the other returns None, while preserving the successful claim’s running-state
assertions; follow the setup and first-writer-wins expectations from
test_claim_run_for_execution_is_first_writer_wins.
In `@tests/generation_run_launcher_support.py`:
- Line 3: Remove the redundant from __future__ import annotations from
tests/generation_run_launcher_support.py at lines 3-3 and
tests/steps/no_qa_generation_slice_support.py at lines 3-3; leave the
TYPE_CHECKING-guarded imports and remaining support-module logic unchanged.
In `@tests/steps/no_qa_generation_slice_support.py`:
- Line 38: Replace the module-level RunResult TypeVar with PEP 695 bracketed
generic syntax on the run function, matching the existing require[RequiredValue]
convention; remove the standalone TypeVar declaration and update run’s generic
annotation accordingly.
In `@tests/test_env_runtime_wiring.py`:
- Around line 104-109: Replace each bare assertion in the specified
integration-test sites with assert …, "message" diagnostics: in
tests/test_env_runtime_wiring.py lines 104-109 cover launcher and cost-recorder
wiring, and lines 160-165 cover captured dependencies and shutdown hooks; in
tests/test_episode_tei_api.py lines 63-83 cover response status, body, and
headers; in tests/test_generation_run_api.py lines 76-89 cover replay, polling,
and event pagination, and lines 128-132 cover conflict and validation responses.
Use concise messages identifying the violated HTTP or wiring contract at every
listed site.
- Around line 355-356: Update the UP037 suppressions on the request and response
annotations in the affected test helper to include an inline justification that
the quoted types remain type-only imports and must not become runtime
dependencies; keep the suppressions narrow and unchanged otherwise.
In `@tests/test_generation_run_port_contract.py`:
- Around line 336-341: In the test assertion block, move the count assertion
before the `claimed = next(...)` assignment so the uniqueness/claim-existence
check runs first. Keep the existing `next(...)` selection and subsequent status,
node, and timestamp assertions unchanged.
---
Outside diff comments:
In `@episodic/canonical/storage/repositories.py`:
- Around line 171-217: Remove the duplicate SqlAlchemyIngestionJobRepository
implementation from episodic/canonical/storage/repositories.py lines 171-217.
Retain the canonical implementation in
episodic/canonical/storage/ingestion_job_repositories.py lines 26-65; no direct
change is needed there, and existing imports in uow.py and __init__.py should
continue referencing it.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9479d14a-6d07-478f-9dc6-30ee466a29ca
📒 Files selected for processing (70)
alembic/versions/20260624_000010_add_generation_run_tables.pyalembic/versions/20260624_000011_add_episode_tei_revisioning.pycharts/episodic/README.mddocs/adr/adr-009-source-to-script-rest-vertical-slice.mddocs/adr/adr-016-no-qa-generation-run-execution-and-tei-persistence.mddocs/contents.mddocs/developers-guide.mddocs/episodic-podcast-generation-system-design.mddocs/execplans/4-3-2-no-qa-generation-runs-and-tei-p5-retrieval.mddocs/repository-layout.mddocs/roadmap.mddocs/users-guide.mdepisodic/api/app.pyepisodic/api/dependencies.pyepisodic/api/resources/__init__.pyepisodic/api/resources/episode_tei.pyepisodic/api/resources/generation_runs.pyepisodic/api/runtime.pyepisodic/api/serializers.pyepisodic/api/source_idempotency.pyepisodic/canonical/__init__.pyepisodic/canonical/adapters/generation_checkpoints.pyepisodic/canonical/adapters/generation_runs.pyepisodic/canonical/domain.pyepisodic/canonical/entity_protocols.pyepisodic/canonical/episode_errors.pyepisodic/canonical/generation_persistence.pyepisodic/canonical/generation_quality.pyepisodic/canonical/generation_run_ports.pyepisodic/canonical/hashing.pyepisodic/canonical/storage/__init__.pyepisodic/canonical/storage/entity_mappers.pyepisodic/canonical/storage/entity_models.pyepisodic/canonical/storage/episode_repository.pyepisodic/canonical/storage/generation_run_models.pyepisodic/canonical/storage/generation_runs.pyepisodic/canonical/storage/ingestion_job_repositories.pyepisodic/canonical/storage/models.pyepisodic/canonical/storage/models_base.pyepisodic/canonical/storage/repositories.pyepisodic/canonical/storage/uow.pyepisodic/canonical/unit_of_work_protocols.pyepisodic/generation/__init__.pyepisodic/generation/draft_script.pyepisodic/generation/launcher.pyepisodic/generation/launcher_support.pypyproject.tomltests/__snapshots__/test_draft_script_generation.ambrtests/__snapshots__/test_generation_run_domain.ambrtests/canonical_storage/test_episode_tei_updates.pytests/canonical_storage/test_generation_runs.pytests/features/no_qa_generation_slice.featuretests/generation_run_launcher_support.pytests/steps/generation_orchestration_vidaimock.pytests/steps/no_qa_generation_slice_support.pytests/steps/test_generation_run_lifecycle_steps.pytests/steps/test_no_qa_generation_slice.pytests/test_draft_script_generation.pytests/test_env_runtime_wiring.pytests/test_episode_tei_api.pytests/test_generation_persistence.pytests/test_generation_run_api.pytests/test_generation_run_domain.pytests/test_generation_run_launcher.pytests/test_generation_run_port_contract.pytests/test_generation_run_properties.pytests/test_protocol_stubs.pytests/test_workflow_test_utils.pytests/workflow_test_utils.pytypos.toml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/hecate(auto-detected)leynos/femtologging(auto-detected)leynos/tei-rapporteur(auto-detected)leynos/falcon-correlate(auto-detected)leynos/shared-actions(auto-detected) → reviewed against branch4-3-2-no-qa-generation-runs-and-tei-p5-retrievalinstead of the default branch
| - import-boundary enforcement model. | ||
| - [ADR 015: Upload and idempotency ports](adr/adr-015-upload-and-idempotency-ports.md) | ||
| - source-intake upload storage and idempotency port decisions. | ||
| - [ADR 016: No-QA generation execution and TEI persistence](adr/adr-016-no-qa-generation-run-execution-and-tei-persistence.md) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the added ADR links at 80 columns.
Use reference-style links or another wrapped form that preserves each ADR title
and target.
docs/contents.md#L100-L100: wrap the new ADR 016 index entry.docs/developers-guide.md#L25-L25: wrap the new ADR 016 decision link.
Triage: [type:docstyle]
📍 Affects 2 files
docs/contents.md#L100-L100(this comment)docs/developers-guide.md#L25-L25
🤖 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/contents.md` at line 100, Wrap the ADR 016 links to remain within 80
columns while preserving their titles and targets. Update docs/contents.md lines
100-100 and docs/developers-guide.md lines 25-25, using reference-style links or
another equivalent wrapped format at both sites.
Sources: Coding guidelines, Path instructions
| As an integration client | ||
| I want to generate a draft script without QA and download the TEI | ||
| So that I can validate the source-to-script workflow over REST | ||
|
|
||
| Background: | ||
| Given a Vidai Mock inference server is running | ||
| And a series profile exists | ||
| And a host presenter profile and a guest presenter profile are bound | ||
|
|
||
| Scenario: Draft generation without QA produces a downloadable TEI-P5 script | ||
| Given an ingestion job with an attached source document | ||
| When I create a draft-without-qa generation run for the ingested episode | ||
| Then the run creation responds 202 Accepted with a Location header | ||
| And the response carries a Retry-After header | ||
| And the run is created with qa_status "skipped" and my rationale recorded |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove first-person wording from the embedded feature.
Replace the As an integration client / I want / So that I can narrative and
my rationale with impersonal wording. This is Markdown, not a .feature
file, so the BDD exception does not apply.
Triage: [type:docstyle]
As per path instructions, “Avoid 2nd person or 1st person pronouns ("I", "you", "we"), exceptions: README.md, BDD .feature files.”
🤖 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/execplans/4-3-2-no-qa-generation-runs-and-tei-p5-retrieval.md` around
lines 1010 - 1024, Update the embedded feature narrative to use impersonal
wording instead of “As an integration client,” “I want,” and “So that I can.”
Also replace “my rationale” in the scenario outcome with neutral wording while
preserving the intended behavior and meaning.
Source: Path instructions
| @@ -0,0 +1,349 @@ | |||
| """Falcon resources for no-QA generation-run creation and polling.""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file ==\n'
cat -n episodic/api/resources/generation_runs.py
printf '\n== version/config search ==\n'
rg -n --hidden --glob '!**/.git/**' 'python_requires|requires-python|target-version|python_version|from __future__ import annotations|GenerationRunLauncher|UowFactory|JsonPayload|cabc.Callable' pyproject.toml .python-version setup.cfg setup.py tox.ini pytest.ini mypy.ini ruff.toml . 2>/dev/null | sed -n '1,220p'Repository: leynos/episodic
Length of output: 35425
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n episodic/api/resources/generation_runs.py | sed -n '1,200p'Repository: leynos/episodic
Length of output: 8432
Remove from __future__ import annotations.
This module targets Python 3.14, so keep UowFactory, JsonPayload, GenerationRunLauncher, and cabc.Callable usable without deferred annotations.
🤖 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 `@episodic/api/resources/generation_runs.py` at line 3, Remove the `from
__future__ import annotations` statement from the module so annotations for
`UowFactory`, `JsonPayload`, `GenerationRunLauncher`, and `cabc.Callable` are
evaluated normally under Python 3.14.
Source: Coding guidelines
| def _required_setting( | ||
| environment: cabc.Mapping[str, str], | ||
| name: str, | ||
| error_message: str, | ||
| ) -> str: | ||
| """Return a required, non-empty environment setting.""" | ||
| value = environment.get(name, "").strip() | ||
| if not value: | ||
| raise RuntimeError(error_message) | ||
| return value | ||
|
|
||
|
|
||
| def _llm_settings( | ||
| environment: cabc.Mapping[str, str], | ||
| ) -> tuple[str | None, str | None]: | ||
| """Return the optional, paired OpenAI-compatible provider settings.""" | ||
| base_url = environment.get("OPENAI_BASE_URL", "").strip() or None | ||
| api_key = environment.get("OPENAI_API_KEY", "").strip() or None | ||
| if (base_url is None) != (api_key is None): | ||
| msg = "OPENAI_BASE_URL and OPENAI_API_KEY must be configured together." | ||
| raise RuntimeError(msg) | ||
| return base_url, api_key | ||
|
|
||
|
|
||
| def _load_runtime_config( | ||
| environ: cabc.Mapping[str, str] | None = None, | ||
| ) -> RuntimeConfig: | ||
| """Read and validate runtime configuration from environment variables.""" | ||
| environment = os.environ if environ is None else environ | ||
| database_url = environment.get("DATABASE_URL", "").strip() | ||
| if not database_url: | ||
| msg = "DATABASE_URL must be set before starting the HTTP service." | ||
| raise RuntimeError(msg) | ||
| object_store_root = environment.get("SOURCE_INTAKE_OBJECT_STORE_ROOT", "").strip() | ||
| if not object_store_root: | ||
| database_url = _required_setting( | ||
| environment, | ||
| "DATABASE_URL", | ||
| "DATABASE_URL must be set before starting the HTTP service.", | ||
| ) | ||
| try: | ||
| object_store_root = _required_setting( | ||
| environment, | ||
| "SOURCE_INTAKE_OBJECT_STORE_ROOT", | ||
| "SOURCE_INTAKE_OBJECT_STORE_ROOT must be set before starting " | ||
| "the HTTP service.", | ||
| ) | ||
| except RuntimeError: | ||
| log_warning( | ||
| logger, | ||
| "runtime_config_missing setting=%s", | ||
| "SOURCE_INTAKE_OBJECT_STORE_ROOT", | ||
| ) | ||
| msg = ( | ||
| "SOURCE_INTAKE_OBJECT_STORE_ROOT must be set before starting " | ||
| "the HTTP service." | ||
| raise |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Raise a configuration-specific error.
Replace these generic RuntimeError paths with ValueError or a dedicated
configuration error, and catch that exact type at Lines 113-126. Preserve
runtime faults as distinct failures.
As per coding guidelines, “Raise specific built-in exceptions or domain-specific exceptions instead of generic Exception or catch-all RuntimeError”.
🤖 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 `@episodic/api/runtime.py` around lines 79 - 126, Replace the generic
RuntimeError exceptions raised by _required_setting and _llm_settings with
ValueError or a dedicated configuration-specific exception. Update the handler
in _load_runtime_config to catch that exact configuration error type while
preserving the existing warning and re-raise behavior; do not catch unrelated
runtime faults.
Source: Coding guidelines
| def serialize_tei_envelope(episode: CanonicalEpisode) -> dict[str, typ.Any]: | ||
| """Serialize generated TEI metadata using the public field names.""" | ||
| return { | ||
| "episode_id": str(episode.id), | ||
| "tei_header_id": str(episode.tei_header_id), | ||
| "tei_xml": episode.tei_xml, | ||
| "content_hash": episode.tei_content_hash, | ||
| "version": episode.tei_revision, | ||
| "last_generation_run_id": _optional_uuid_str(episode.last_generation_run_id), | ||
| "quality_mode": "draft_without_qa", | ||
| "qa_status": None if episode.qa_status is None else episode.qa_status.value, | ||
| "updated_at": episode.updated_at.isoformat(), | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reference QualityMode.DRAFT_WITHOUT_QA instead of hardcoding its wire value.
Stop duplicating the enum's string value here. serialize_generation_run correctly reads run.quality_mode.value; make serialize_tei_envelope do the same by importing the enum, so the two serializers cannot drift apart if the wire value ever changes.
♻️ Proposed fix
+from episodic.canonical.generation_quality import QualityMode
+
def serialize_tei_envelope(episode: CanonicalEpisode) -> dict[str, typ.Any]:
"""Serialize generated TEI metadata using the public field names."""
return {
"episode_id": str(episode.id),
"tei_header_id": str(episode.tei_header_id),
"tei_xml": episode.tei_xml,
"content_hash": episode.tei_content_hash,
"version": episode.tei_revision,
"last_generation_run_id": _optional_uuid_str(episode.last_generation_run_id),
- "quality_mode": "draft_without_qa",
+ "quality_mode": QualityMode.DRAFT_WITHOUT_QA.value,
"qa_status": None if episode.qa_status is None else episode.qa_status.value,
"updated_at": episode.updated_at.isoformat(),
}📝 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.
| def serialize_tei_envelope(episode: CanonicalEpisode) -> dict[str, typ.Any]: | |
| """Serialize generated TEI metadata using the public field names.""" | |
| return { | |
| "episode_id": str(episode.id), | |
| "tei_header_id": str(episode.tei_header_id), | |
| "tei_xml": episode.tei_xml, | |
| "content_hash": episode.tei_content_hash, | |
| "version": episode.tei_revision, | |
| "last_generation_run_id": _optional_uuid_str(episode.last_generation_run_id), | |
| "quality_mode": "draft_without_qa", | |
| "qa_status": None if episode.qa_status is None else episode.qa_status.value, | |
| "updated_at": episode.updated_at.isoformat(), | |
| } | |
| from episodic.canonical.generation_quality import QualityMode | |
| def serialize_tei_envelope(episode: CanonicalEpisode) -> dict[str, typ.Any]: | |
| """Serialize generated TEI metadata using the public field names.""" | |
| return { | |
| "episode_id": str(episode.id), | |
| "tei_header_id": str(episode.tei_header_id), | |
| "tei_xml": episode.tei_xml, | |
| "content_hash": episode.tei_content_hash, | |
| "version": episode.tei_revision, | |
| "last_generation_run_id": _optional_uuid_str(episode.last_generation_run_id), | |
| "quality_mode": QualityMode.DRAFT_WITHOUT_QA.value, | |
| "qa_status": None if episode.qa_status is None else episode.qa_status.value, | |
| "updated_at": episode.updated_at.isoformat(), | |
| } |
🤖 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 `@episodic/api/serializers.py` around lines 247 - 259, Update
serialize_tei_envelope to import and reference
QualityMode.DRAFT_WITHOUT_QA.value for quality_mode instead of hardcoding
"draft_without_qa". Keep the remaining envelope fields unchanged and align this
serializer with serialize_generation_run.
| @@ -0,0 +1,313 @@ | |||
| """Support fixtures for generation-run launcher tests.""" | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the redundant from __future__ import annotations from both new test-support modules. Neither file matches the tests/steps/test_*_steps.py retention exception, and the Python 3.14 baseline already defers annotation evaluation by default — exactly the reasoning already applied earlier in this PR to strip the same import from episodic/generation/launcher.py and episodic/generation/launcher_support.py.
tests/generation_run_launcher_support.py#L3-L3: delete thefrom __future__ import annotationsline; TYPE_CHECKING-guarded SQLAlchemy imports remain safe since annotations are lazily evaluated.tests/steps/no_qa_generation_slice_support.py#L3-L3: delete thefrom __future__ import annotationsline for the same reason.
📍 Affects 2 files
tests/generation_run_launcher_support.py#L3-L3(this comment)tests/steps/no_qa_generation_slice_support.py#L3-L3
🤖 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 `@tests/generation_run_launcher_support.py` at line 3, Remove the redundant
from __future__ import annotations from tests/generation_run_launcher_support.py
at lines 3-3 and tests/steps/no_qa_generation_slice_support.py at lines 3-3;
leave the TYPE_CHECKING-guarded imports and remaining support-module logic
unchanged.
Source: Coding guidelines
| from episodic.api.dependencies import ApiDependencies | ||
|
|
||
|
|
||
| RunResult = typ.TypeVar("RunResult") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a bracketed generic instead of a module-level TypeVar.
require in this same file already uses PEP 695 bracket syntax (def require[RequiredValue](...)). Match that convention for run and drop the standalone TypeVar.
♻️ Proposed fix
-RunResult = typ.TypeVar("RunResult")
-
-
`@dc.dataclass`(slots=True)
class NoQaGenerationSliceContext:
...
- def run(self, operation: cabc.Awaitable[RunResult]) -> RunResult:
+ def run[RunResult](self, operation: cabc.Awaitable[RunResult]) -> RunResult:
"""Run an asynchronous scenario operation on the shared event loop."""
return self.runner.run(operation)Also applies to: 68-70
🤖 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 `@tests/steps/no_qa_generation_slice_support.py` at line 38, Replace the
module-level RunResult TypeVar with PEP 695 bracketed generic syntax on the run
function, matching the existing require[RequiredValue] convention; remove the
standalone TypeVar declaration and update run’s generic annotation accordingly.
Source: Coding guidelines
| assert isinstance(launcher, InProcessGenerationRunLauncher) | ||
| assert launcher.cost_recorder_factory is not None | ||
| async with SqlAlchemyUnitOfWork(factory) as uow: | ||
| recorder = launcher.cost_recorder_factory(uow) | ||
| assert isinstance(recorder, CostRecorder) | ||
| assert recorder.ledger is uow.cost_ledger |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add diagnostic messages to bare assertions.
Add concise failure messages consistently so failed integration assertions identify
the violated HTTP or wiring contract.
tests/test_env_runtime_wiring.py#L104-L109: add messages for launcher and
cost-recorder assertions.tests/test_env_runtime_wiring.py#L160-L165: add messages for captured
dependency and shutdown-hook assertions.tests/test_episode_tei_api.py#L63-L83: add messages for status, body, and
response-header assertions.tests/test_generation_run_api.py#L76-L89: add messages for replay, polling,
and event-pagination assertions.tests/test_generation_run_api.py#L128-L132: add messages for conflict and
validation response assertions.
As per coding guidelines and path instructions, use assert …, "message" over bare assertions.
📍 Affects 3 files
tests/test_env_runtime_wiring.py#L104-L109(this comment)tests/test_env_runtime_wiring.py#L160-L165tests/test_episode_tei_api.py#L63-L83tests/test_generation_run_api.py#L76-L89tests/test_generation_run_api.py#L128-L132
🤖 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 `@tests/test_env_runtime_wiring.py` around lines 104 - 109, Replace each bare
assertion in the specified integration-test sites with assert …, "message"
diagnostics: in tests/test_env_runtime_wiring.py lines 104-109 cover launcher
and cost-recorder wiring, and lines 160-165 cover captured dependencies and
shutdown hooks; in tests/test_episode_tei_api.py lines 63-83 cover response
status, body, and headers; in tests/test_generation_run_api.py lines 76-89 cover
replay, polling, and event pagination, and lines 128-132 cover conflict and
validation responses. Use concise messages identifying the violated HTTP or
wiring contract at every listed site.
Sources: Coding guidelines, Path instructions
| request: "LLMRequest", # noqa: UP037 | ||
| ) -> "LLMResponse": # noqa: UP037 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Justify the lint suppressions inline.
Append a reason to each # noqa: UP037, explaining that the quoted types remain
type-only imports and must not become runtime dependencies.
As per path instructions, “Only narrow in-line disables (# noqa: XYZ) are permitted, must be accompanied by justification and used only as a last resort.”
🤖 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 `@tests/test_env_runtime_wiring.py` around lines 355 - 356, Update the UP037
suppressions on the request and response annotations in the affected test helper
to include an inline justification that the quoted types remain type-only
imports and must not become runtime dependencies; keep the suppressions narrow
and unchanged otherwise.
| claimed = next(result for result in results if result is not None) | ||
|
|
||
| assert sum(result is not None for result in results) == 1 | ||
| assert claimed.status is GenerationRunStatus.RUNNING | ||
| assert claimed.current_node == "draft" | ||
| assert claimed.started_at == NOW |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert before calling next() to avoid masking a real failure with StopIteration.
If both claims ever come back None, next() on the empty generator raises StopIteration before the informative sum(...) == 1 assertion runs. Reorder so the assertion fires first and gives a clear failure message.
🐛 Proposed fix
- claimed = next(result for result in results if result is not None)
-
- assert sum(result is not None for result in results) == 1
+ assert sum(result is not None for result in results) == 1
+ claimed = next(result for result in results if result is not None)📝 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.
| claimed = next(result for result in results if result is not None) | |
| assert sum(result is not None for result in results) == 1 | |
| assert claimed.status is GenerationRunStatus.RUNNING | |
| assert claimed.current_node == "draft" | |
| assert claimed.started_at == NOW | |
| assert sum(result is not None for result in results) == 1 | |
| claimed = next(result for result in results if result is not None) | |
| assert claimed.status is GenerationRunStatus.RUNNING | |
| assert claimed.current_node == "draft" | |
| assert claimed.started_at == NOW |
🤖 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 `@tests/test_generation_run_port_contract.py` around lines 336 - 341, In the
test assertion block, move the count assertion before the `claimed = next(...)`
assignment so the uniqueness/claim-existence check runs first. Keep the existing
`next(...)` selection and subsequent status, node, and timestamp assertions
unchanged.
2 participants
Summary
This implementation completes roadmap task 4.3.2 — No-QA generation runs
and TEI-P5 retrieval, the second half of the source-to-script REST vertical
slice defined in ADR 009.
The design and build phases are complete. The living ExecPlan records all
milestone decisions, discoveries, progress, and validation evidence:
docs/execplans/4-3-2-no-qa-generation-runs-and-tei-p5-retrieval.mdImplemented behaviour
An integration client can now:
quality_mode=draft_without_qa, a rationale,and actor metadata.
application/tei+xmlwith an ETagand attachment metadata.
Identical idempotent replays preserve the run id,
Location, andRetry-After; changed bodies conflict. Provider and TEI failures becomeclassified terminal run state.
Architecture
GenerationRunLauncherisolates scheduling from durable execution state; thefirst adapter runs in-process with bounded concurrency and shutdown draining.
persist through SQLAlchemy adapters and Alembic migrations.
DraftScriptGeneratorisolates the single-pass LLM draft policy from thelauncher and its future roadmap 4.4.1 successor.
recovery hooks, HTTP status choices, and content negotiation.
Validation
make check-fmtmake typecheckmake lint— Pylint 10.00/10make check-migrationsmake test— 1,076 passed, 1 skippedmake markdownlintmake nixievidaimock 0.1.3References