Complete source and presenter-profile intake (4.3.1) - #137
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Roadmap 4.3.1 completion: source-and-presenter-profile intake idempotency and runtime wiringThis PR closes Roadmap item 4.3.1 by completing the source-and-presenter-profile intake slice with idempotent multipart upload handling, object-store runtime wiring, and supporting API extensions. Core functionality: runtime environment and idempotencyObject-store wiring: The Idempotency scoping by principal: The Multi-phase upload persistence: SQLAlchemy idempotency with first-writer-wins: API contract extensionsTwo new
Both endpoints are registered in the app ( Code quality: refactoring and consolidationLarge-method extraction:
Test helper consolidation: Five CodeScene duplication warnings in test files were eliminated by extracting shared BDD setup patterns. In Testing: property, integration, and contract validationProperty-based tests (
Integration tests validate idempotency under concurrency (both acquisitions in Contract-driven tests ( Documentation and design decisionsThe ExecPlan ( The developers-guide ( The users-guide ( The roadmap ( Observability and injectable dependenciesStructured logs are emitted at decision boundaries (pending-commit, object-stored, ready, ready-commit-failure) with operation labels only (no sensitive principal/key data). Bounded counters and latency metrics use explicit outcome labels. Time and randomness providers ( Configuration and defaultsThe default maximum upload size ( Validation and reviewAll 870 tests pass (2 skipped); formatting, markdown linting, diagram validation, type checking, and linting all pass. CodeRabbit review completed with zero findings after PR-feedback fixes. The worklist tracked eight categories of findings (code health, test coverage, architecture, documentation, property testing, observability, and test consolidation), all addressed in the final state. WalkthroughUpdate roadmap 4.3.1 intake and idempotency guidance, wire request-scoped principals and object-store startup requirements, refactor upload persistence and idempotency storage into staged transactions, and extend the API, storage, and test coverage for uploads and source listings. ChangesUpload idempotency and intake wiring
Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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.
|
@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. episodic/canonical/storage/source_intake_repositories.py Comment on file request: IdempotencyAcquireRequest,
) -> IdempotencyOutcome:
"""Acquire or inspect an idempotency record."""
started_at = self._runtime.monotonic_clock.monotonic_seconds()❌ New issue: Large Method |
|
@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. episodic/canonical/source_intake_service.py Comment on file async def register_upload(
uow: CanonicalUnitOfWork,
uow_factory: UowFactory,❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== tests/test_env_runtime_wiring.py ==="
cat tests/test_env_runtime_wiring.py
echo ""
echo "=== episodic/canonical/source_intake_service.py ==="
cat episodic/canonical/source_intake_service.py
echo ""
echo "=== episodic/canonical/storage/source_intake_repositories.py (acquire method area) ==="
rg -n "datetime|uuid4|monotonic|acquire|clock|id_factory" episodic/canonical/storage/source_intake_repositories.py -A2 -B2
echo ""
echo "=== docs/users-guide.md (source-to-script section) ==="
rg -n "principal|409|Conflict|idempoten|scoped|authoris" docs/users-guide.md -A3 -B3
echo ""
echo "=== Hypothesis / property-based tests ==="
rg -rn "`@given`\|hypothesis\|from hypothesis" tests/
echo ""
echo "=== Observability: logger/metrics in service and repositories ==="
rg -n "logger\|logging\|structlog\|metrics\|span\|trace\|instrument" episodic/canonical/source_intake_service.py episodic/canonical/storage/source_intake_repositories.py episodic/api/runtime.pyLength of output: 27340 🏁 Script executed: #!/bin/bash
echo "=== episodic/api/runtime.py ==="
cat episodic/api/runtime.py
echo ""
echo "=== Property-based tests (full scan) ==="
rg -rn "given|hypothesis" tests/ --type py -iLength of output: 18142 🏁 Script executed: #!/bin/bash
echo "=== tests/test_idempotency_properties.py ==="
cat tests/test_idempotency_properties.py
echo ""
echo "=== git log for test_idempotency_properties.py ==="
git log --oneline tests/test_idempotency_properties.py | head -10Length of output: 7385
✅ Testing (Overall) — Resolved
stored_path = object_store_root / "uploads" / response_body["id"]
assert response_body["content_hash"] == f"sha256:{expected_hash}"
assert stored_path.is_file(), f"expected upload payload at {stored_path}"
assert stored_path.read_bytes() == payloadThe oracle on ✅ Unit Architecture — ResolvedBoth command paths now use injectable abstractions instead of bare calls:
✅ User-Facing Documentation — Resolved
Both principal-scoping and the conflict behaviour are documented. ✅ Testing (Property / Proof) — Resolved
Upload-persistence failure invariants remain covered only by example-based tests ( ✅ Observability — Resolved
log_warning(logger, "runtime_config_missing setting=%s", "SOURCE_INTAKE_OBJECT_STORE_ROOT")
# ... RuntimeError raised
log_info(logger, "runtime_config_loaded source_intake_object_store_root=%s", object_store_root)
All originally failing checks are addressed. No further agent prompts are required for this PR. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
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. Excess Number of Function Argumentsepisodic/canonical/source_intake_service.py: _store_upload_bytes What lead to degradation?_store_upload_bytes has 5 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. Excess Number of Function Argumentsepisodic/canonical/source_intake_service.py: _commit_ready_upload What lead to degradation?_commit_ready_upload 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. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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. Comment on lines +150 to +154 def _psycopg_connection_kwargs(url: URL) -> PsycopgConnectKwargs:
"""Return Psycopg connection kwargs without rendering secrets into a URL."""
connection_kwargs = url.translate_connect_args(
username="user",
database="dbname",❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Introduce a private app configuration parameter object for source-intake BDD helper calls so `_run_source_intake_call` stays within the argument threshold while preserving the existing ASGI dependency wiring.
|
@coderabbitai resume |
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. tests/steps/source_intake_support.py Comment on lines +77 to +100 def upload_unsupported_content_type(
context: SourceIntakeContext,
session_factory: async_sessionmaker[AsyncSession],
tmp_path: Path,
) -> None:
"""Upload source material whose declared content type is not allowlisted."""
async def _action(
client: httpx.AsyncClient,
scenario_context: SourceIntakeContext,
) -> None:
response = await post_text_upload(
client,
key="bdd-unsupported-content-type",
payload=b"source\n",
content_type="application/octet-stream",
)
record_error_response(response, scenario_context)
_run_source_intake_call(
context,
_SourceIntakeAppConfig(session_factory=session_factory, tmp_path=tmp_path),
_action,
)❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract the shared single-error-call wrapper used by source-intake BDD steps so the public helpers only describe the request they exercise while centralising error recording and app invocation.
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. tests/steps/source_intake_support.py Comment on lines +77 to +102 def _run_single_error_call(
context: SourceIntakeContext,
session_factory: async_sessionmaker[AsyncSession],
tmp_path: Path,
request_fn: cabc.Callable[[httpx.AsyncClient], cabc.Awaitable[httpx.Response]],
*,
upload_max_bytes: int | None = None,
) -> None:
"""Run a single HTTP action, recording the error response into context."""
async def _action(
client: httpx.AsyncClient,
scenario_context: SourceIntakeContext,
) -> None:
response = await request_fn(client)
record_error_response(response, scenario_context)
_run_source_intake_call(
context,
_SourceIntakeAppConfig(
session_factory=session_factory,
tmp_path=tmp_path,
upload_max_bytes=upload_max_bytes,
),
_action,
)❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
Route all single-error source-intake BDD wrappers through the shared helper so response recording and app invocation live in one place while preserving the existing request-specific closures.
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. tests/steps/source_intake_support.py Comment on lines +176 to +196 def attach_missing_upload(
context: SourceIntakeContext,
session_factory: async_sessionmaker[AsyncSession],
tmp_path: Path,
) -> None:
"""Attach an unknown upload to a known ingestion job."""
async def _request(client: httpx.AsyncClient) -> httpx.Response:
profile_id = await create_series_profile(client)
job_id = await create_ingestion_job(client, profile_id)
return await client.post(
f"/v1/ingestion-jobs/{job_id}/sources",
headers={"Idempotency-Key": "bdd-missing-upload"},
json=upload_payload(str(uuid.uuid4())),
)
_run_single_error_call(
context,
_SourceIntakeAppConfig(session_factory=session_factory, tmp_path=tmp_path),
_request,
)❌ New issue: Code Duplication |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@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. Code Duplicationtests/steps/source_intake_support.py: What lead to degradation?The module contains 2 functions with similar structure: attach_missing_upload,attach_pending_upload Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
@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. Code Duplicationtests/test_source_intake_api_contract.py: What lead to degradation?The module contains 3 functions with similar structure: test_attach_source_reports_missing_ingestion_job,test_attach_upload_reports_missing_upload,test_attach_upload_reports_not_ready_upload Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Share the profile, ingestion job, and upload attachment request setup between BDD error-path helpers while keeping the caller-specific upload id sources explicit.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Share the repeated source attachment POST call in API contract tests while leaving each scenario's setup and assertions explicit.
2 participants
Summary
This branch completes the Roadmap 4.3.1 follow-up after the post-merge completeness assessment of the source and presenter-profile intake slice. It closes the remaining runtime, durability, concurrency, observability, property-testing, and documentation gaps so the implemented direct multipart intake workflow can be marked complete.
Roadmap task: (4.3.1)
Execplan: docs/execplans/4-3-1-source-and-presenter-profile-intake-script-generation.md
The branch implements the planned completion by wiring the runtime object store, binding idempotency to the authenticated principal, making upload persistence recoverable across object-store writes, making SQLAlchemy idempotency acquisition first-writer-wins under concurrency, and closing the roadmap and guide documentation. Follow-up review feedback is also addressed: object-store tests now assert persisted bytes, source-intake command paths use injectable time and identifier providers, idempotency has an added principal-scoping property, and bounded logs and metrics are emitted at the new decision boundaries. The deliberate scope decision is unchanged:
POST /v1/uploads/initremains deferred, whilePOST /v1/uploadsis the shipped direct multipart upload contract.Review walkthrough
SOURCE_INTAKE_OBJECT_STORE_ROOTruntime requirement, defaultFilesystemObjectStoredependency wiring, and runtime configuration logging.Validation
make check-fmt: passed.make markdownlint: passed with zero errors.make nixie: passed; all diagrams validated successfully.make build: passed before the PR-feedback follow-up.make check-migrations: passed before the PR-feedback follow-up; Alembic migration drift check completed.make lint: passed with Hecate, Ruff, and Pylint clean.make typecheck: passed with ty 0.0.32.make test: passed with 870 passed and 2 skipped.coderabbit review --agent --type uncommitted: passed for the code milestone with zero findings.coderabbit review --agent --type uncommitted: passed for the documentation milestone with zero findings.coderabbit review --agent --type uncommitted: passed after the final ExecPlan update with zero findings.coderabbit review --agent --type uncommitted: passed after the PR-feedback fixes with zero findings.coderabbit review --agent --type uncommitted: passed after the PR-feedback ExecPlan note with zero findings.Notes
POST /v1/uploads/initremains explicitly deferred. The shipped contract is direct multipart upload throughPOST /v1/uploads; a later pre-signed upload flow can extend the same upload and idempotency ports without changing the current public contract.