Slice 2 — US1 MVP (Phase 3, T017-T023)#6
Conversation
Stack on PR #3. Wires the end-to-end write-enabled loop against the in-process Dataverse fake — the constitution's CRM-first principle made operational. - T018: DataverseWriteBackAdapter — emit_phone_call_activity / emit_queue_status_update / emit_task / build_writeback against the Dataverse Web API, with FR-024 idempotency pre-query, FR-003 approved-update-field gating, FR-025 task-owner resolution (default + verified override + fallback warning), and crm_correlations / writeback_progress ledger writes. - T019: slice2/runner.py write-enabled coordinator — readiness gate, mapping approval check, verify() metadata gate, DataverseQueueLoader, stages the queue row locally, calls the unchanged process_one_queue_item (FR-014), stamps writeback_progress.run_status, maps to exit_status per cli-slice2.md. - T020: discover-crm CLI command — refreshes the mapping artifact via discover() and rewrites config/dataverse_mapping.json with approved=false. - T021: run-crm CLI command — explicit --write flag; reports exit_status / session_id / final_disposition / warnings. - T022: FR-034 non-E.164 phone warning — recorded into CrmRunReport and appended to queue.last_error via the adapter, never changing exit status. - T017: contract test — adapter satisfies the unchanged WriteBackAdapter Protocol and produces the right Dataverse interactions for every one of the 11 final dispositions (SC-011). - T023: US1 integration tests — happy path, interested_email_captured, needs_human_review, do_not_call, blocked, non-E.164 warning, and empty-queue no-op (SC-001 / SC-003 / SC-004 / SC-008). ruff + pytest pass (270 tests; pre-existing failures in test_constitution_sync.py inherited from fab3ebd are unrelated to this PR). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer's GuideImplements the Slice 2 Dataverse-backed write-enabled path, adding new CLI subcommands for CRM discovery and single-item processing, a Dataverse-specific write-back adapter and runner, and comprehensive contract and integration tests, while wiring non-E.164 phone warnings into run reports and queue updates. Sequence diagram for run-crm single-item processingsequenceDiagram
actor User
participant CLI as run_crm
participant Config as load_config
participant Slice2Config as load_slice2_config
participant Secrets as load_dataverse_secrets
participant Store as store
participant Runner as run_one_crm_item
participant DVClient as DataverseClient
participant Loader as DataverseQueueLoader.load
participant Orchestrator as process_one_queue_item
participant Adapter as DataverseWriteBackAdapter
User->>CLI: run-crm --write --next-ready
CLI->>CLI: _build_selector
CLI->>Config: load_config
CLI->>Slice2Config: load_slice2_config
CLI->>Secrets: load_dataverse_secrets
CLI->>DVClient: DataverseClient.__init__
CLI->>Store: store.connect
CLI->>Runner: run_one_crm_item(selector, transport_fixture,...)
Runner->>Runner: load_mapping
Runner->>Runner: MappingTranslator.__init__
Runner->>DVClient: verify
alt metadata not ok
Runner-->>CLI: CrmRunReport(exit_status=blocked)
CLI-->>User: exit with code mapped from exit_status
Note over Runner,CLI: return
end
Runner->>DVClient: DataverseQueueLoader.load
alt no queue item
Runner-->>CLI: CrmRunReport(exit_status=no-callable-item)
CLI-->>User: exit 0
Note over Runner,CLI: return
end
Runner->>Store: _stage_queue_item (insert_queue_item/update_queue_item_status)
Runner->>Adapter: DataverseWriteBackAdapter.__init__
Runner->>Adapter: add_warning (non_e164_phone)
Runner->>Orchestrator: process_one_queue_item
Orchestrator->>Adapter: emit_phone_call_activity
Orchestrator->>Adapter: emit_queue_status_update
Orchestrator->>Adapter: emit_task
Orchestrator-->>Runner: RunReport
Runner->>Adapter: finalize_progress
Runner-->>CLI: CrmRunReport(exit_status, session_id,...)
CLI-->>User: echo report fields
CLI-->>User: exit with _EXIT_CODE[exit_status]
CLI->>Store: conn.close
CLI->>DVClient: DVClient.close
Sequence diagram for discover-crm metadata refreshsequenceDiagram
actor User
participant CLI as discover_crm
participant Slice2Config as load_slice2_config
participant Secrets as load_dataverse_secrets
participant Mapping as load_mapping
participant DVToken as DataverseTokenProvider
participant DVClient as DataverseClient
participant Discover as discover
participant FS as mapping_artifact_file
User->>CLI: discover-crm --slice2-config --out
CLI->>Slice2Config: load_slice2_config
Slice2Config-->>CLI: Slice2Config
CLI->>Secrets: load_dataverse_secrets
Secrets-->>CLI: DataverseSecrets
CLI->>Mapping: load_mapping(artifact_path)
Mapping-->>CLI: scaffold
CLI->>DVToken: DataverseTokenProvider.__init__
CLI->>DVClient: DataverseClient.__init__
CLI->>Discover: discover(DVClient, scaffold, now_utc_ms)
Discover-->>CLI: refreshed_mapping
CLI->>DVClient: DVClient.close
CLI->>DVToken: DVToken.close
CLI->>FS: write_text(json.dumps(refreshed_mapping))
CLI-->>User: echo discovered path, approved flag, discovered_at
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull request overview
Implements US1 Slice 2 “write-enabled” end-to-end loop against the in-process Dataverse fake, including a Dataverse-backed WriteBackAdapter, a Slice 2 runner that reuses the unchanged Slice 1 orchestrator, and new CLI commands for Dataverse discovery and single-item processing.
Changes:
- Added
DataverseWriteBackAdapterimplementing the Slice 1WriteBackAdapterprotocol using Dataverse Web API semantics (idempotency pre-query, progress/correlation stamping, owner override resolution). - Added Slice 2
run_one_crm_itemrunner to gate on mapping + live metadata verification, load one queue item from Dataverse, run the orchestrator, and stamp terminal progress (including FR-034 non‑E.164 warning propagation). - Added
discover-crmandrun-crmCLI commands, plus contract/integration tests covering disposition write-back behavior and an end-to-end write-enabled happy path against the Dataverse fake.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/integration/test_us1_write_enabled.py |
End-to-end integration coverage for write-enabled loop against Dataverse fake across key dispositions, blocked eligibility, warnings, and empty queue. |
tests/contract/test_dataverse_adapter_contract.py |
Contract tests validating per-disposition emission behavior, idempotency, owner override behavior, and aggregate assembly for the Dataverse adapter. |
tests/contract/__init__.py |
Package marker for contract tests. |
src/opencloser/slice2/runner.py |
Slice 2 runner coordinating readiness checks, queue loading, orchestrator invocation, warnings, and terminal progress stamping. |
src/opencloser/crm/dataverse/adapter.py |
Dataverse-backed implementation of the unchanged Slice 1 write-back protocol with mapping translation, idempotency, correlations, and ownership resolution. |
src/opencloser/cli.py |
Adds Slice 2 CLI commands (discover-crm, run-crm) and exit-status mapping. |
specs/002-mock-call-real-crm/tasks.md |
Marks US1 implementation and tests as completed in the Spec Kit task list. |
| def _owner_bind(owner_id: str) -> str: | ||
| """OData @odata.bind value for a systemuser/team owner reference.""" | ||
| # We default to systemuser binding; verification at owner-resolution time covers | ||
| # the team case (a team-only owner is still a valid `ownerid` lookup). | ||
| return f"/systemusers({owner_id})" | ||
|
|
||
|
|
| default = ( | ||
| self._task_owners.callback | ||
| if payload.task_kind == "callback" | ||
| else self._task_owners.review | ||
| ) | ||
| override = self._lookup_owner_override(payload.queue_item_id) | ||
| if override is None: | ||
| return default | ||
| if self._owner_is_verifiable(override): | ||
| return override |
| write: Annotated[ | ||
| bool, | ||
| typer.Option( | ||
| "--write/--dry-run", | ||
| help="Enable CRM writes (FR-031). Defaults to false — dry-run not yet wired (US2).", | ||
| ), | ||
| ] = False, | ||
| queue_item_id: Annotated[ | ||
| str | None, | ||
| typer.Option("--queue-item-id", help="Dataverse queue-item GUID to process"), | ||
| ] = None, | ||
| next_ready: Annotated[ | ||
| bool, | ||
| typer.Option("--next-ready", help="Process the deterministically-next ready item"), | ||
| ] = False, | ||
| campaign: Annotated[ | ||
| str | None, | ||
| typer.Option("--campaign", help="Campaign selector (overrides slice2.toml [run].campaign)"), | ||
| ] = None, | ||
| transport_fixture: Annotated[ | ||
| Path, | ||
| typer.Option("--transport-fixture", help="Path to a transport-events JSON fixture"), | ||
| ] = ..., # type: ignore[assignment] | ||
| conversation_fixture: Annotated[ | ||
| Path | None, | ||
| typer.Option( | ||
| "--conversation-fixture", | ||
| help="Path to the scripted conversation JSON (required for connected calls)", | ||
| ), | ||
| ] = None, | ||
| config_path: Annotated[ | ||
| Path, typer.Option("--config", help="Path to slice1.toml") | ||
| ] = _DEFAULT_CONFIG_PATH, | ||
| slice2_config_path: Annotated[ | ||
| Path, | ||
| typer.Option("--slice2-config", help="Path to config/slice2.toml"), | ||
| ] = _DEFAULT_SLICE2_CONFIG, | ||
| ) -> None: | ||
| """Process exactly one Dataverse queue item (FR-032). | ||
|
|
||
| Requires `--write` for write-enabled processing. Dry-run is the documented | ||
| default per FR-031 but is wired up in a later slice (US2 / T024-T026); calling | ||
| `run-crm` without `--write` exits 2 with an explanatory message. | ||
| """ | ||
| if not write: | ||
| typer.echo( | ||
| "error: dry-run mode is not yet implemented (US2). Pass --write to run " | ||
| "the write-enabled path.", | ||
| err=True, | ||
| ) | ||
| raise typer.Exit(code=2) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
DataverseWriteBackAdapter._fetch_queue_last_sessionand_lookup_owner_override, the$filterclauses interpolatequeue_item_idwithout quotes, which may be invalid for GUID/string IDs in real Dataverse; consider consistently quoting/escaping ID values when building OData filters. - The logic that composes and inspects
medx_callqueueitemfields (e.g.medx_callstatus,medx_lastsessionid,medx_assignedownerid) is duplicated across the adapter, queue loader, and tests; consider centralizing these logical names/field accessors to reduce drift if the mapping changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DataverseWriteBackAdapter._fetch_queue_last_session` and `_lookup_owner_override`, the `$filter` clauses interpolate `queue_item_id` without quotes, which may be invalid for GUID/string IDs in real Dataverse; consider consistently quoting/escaping ID values when building OData filters.
- The logic that composes and inspects `medx_callqueueitem` fields (e.g. `medx_callstatus`, `medx_lastsessionid`, `medx_assignedownerid`) is duplicated across the adapter, queue loader, and tests; consider centralizing these logical names/field accessors to reduce drift if the mapping changes.
## Individual Comments
### Comment 1
<location path="src/opencloser/crm/dataverse/adapter.py" line_range="521" />
<code_context>
+ return entity_uri[lparen + 1 : rparen].strip("'")
+
+
+def _owner_bind(owner_id: str) -> str:
+ """OData @odata.bind value for a systemuser/team owner reference."""
+ # We default to systemuser binding; verification at owner-resolution time covers
</code_context>
<issue_to_address>
**issue (bug_risk):** Binding all owners as systemusers will be wrong for team owners.
The `@odata.bind` URI is always `/systemusers(...)`, so team owners are bound to the wrong entity set and Dataverse will reject the request. Even if ownership is verified elsewhere, the bind must target `/systemusers(...)` or `/teams(...)` as appropriate. Please derive the correct collection based on whether the verified owner id is a user or a team (for example, by having `_owner_is_verifiable` return a structured type or reusing the entity it queries).
</issue_to_address>
### Comment 2
<location path="src/opencloser/crm/dataverse/adapter.py" line_range="306" />
<code_context>
+ ) -> str:
+ """FR-024 — pre-query for an existing record with this idempotency key; if
+ found, return its id. Otherwise POST and return the new record id."""
+ existing = (
+ self._client.get(
+ entity,
</code_context>
<issue_to_address>
**issue (bug_risk):** Deriving the primary key as `f"{entity}id"` may be incorrect for some entities.
In `_idempotent_create`, the primary key is assumed to be `f"{entity}id"`. For some entities (e.g. activities like phone calls), the logical name and primary id differ (`phonecallid`, `activityid`, etc.). Since `MappingTranslator.mapping.entities[...]` already exposes `primary_id` (as used for queue items), use that metadata to determine the id field instead of deriving it from `entity`, to avoid idempotent create failing to reuse existing records where the convention doesn’t hold.
</issue_to_address>
### Comment 3
<location path="src/opencloser/slice2/runner.py" line_range="64-65" />
<code_context>
+_E164_RE = re.compile(r"^\+[1-9]\d{1,14}$")
+
+
+class ReadinessError(RuntimeError):
+ """Readiness/metadata gate failure — `run-crm` exits before any write (FR-002/FR-007)."""
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider either using `ReadinessError` or removing it to avoid dead code.
`ReadinessError` is defined but never raised or handled. If you want a distinct path for readiness failures (e.g., with specific exit codes), consider wrapping the readiness block (`load_mapping`/`verify`) and raising this explicitly. Otherwise, remove the unused class to keep the module simpler.
Suggested implementation:
```python
_E164_RE = re.compile(r"^\+[1-9]\d{1,14}$")
Wraps the **unchanged** Slice 1 orchestrator (FR-014). Per write-enabled run:
```
```python
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e99cdb87b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """OData @odata.bind value for a systemuser/team owner reference.""" | ||
| # We default to systemuser binding; verification at owner-resolution time covers | ||
| # the team case (a team-only owner is still a valid `ownerid` lookup). | ||
| return f"/systemusers({owner_id})" |
There was a problem hiding this comment.
Task ownership resolution allows overrides that verify as a team, but _owner_bind always emits /systemusers(...). When a queue item carries a valid team owner override (or defaults are configured as team IDs), the created task payload points at the wrong entity type and the write-back can fail at runtime, blocking task creation for those runs.
Useful? React with 👍 / 👎.
| "$filter": f"{entity}id eq {owner_id}", | ||
| "$select": f"{entity}id", | ||
| "$top": "1", |
There was a problem hiding this comment.
The override verifier treats any matching systemuser row as valid, but does not filter out disabled users. In environments where disabled users remain in Dataverse, a stale override can be accepted and written through instead of falling back to the configured default owner, violating the “active enabled user/team” rule and misrouting follow-up tasks.
Useful? React with 👍 / 👎.
| "ownerid@odata.bind": _owner_bind(owner_id), | ||
| } | ||
| if payload.preferred_callback_window: | ||
| body["scheduledend"] = payload.preferred_callback_window |
There was a problem hiding this comment.
This code writes preferred_callback_window directly into scheduledend, but that field is a structured Dataverse datetime while the callback window is intentionally free-form text (e.g., “Thursday afternoon”). Any non-ISO phrase can trigger a 400 on task creation, turning otherwise successful callback outcomes into write failures.
Useful? React with 👍 / 👎.
| store.update_queue_item_status( | ||
| conn, | ||
| queue_item.queue_item_id, | ||
| callable_status=queue_item.callable_status, | ||
| dnc_flag=queue_item.dnc_flag, |
There was a problem hiding this comment.
When a queue item already exists locally, staging only updates callable_status and dnc_flag and leaves other mutable fields (notably attempt_count, phone, and timezone) stale. On repeat runs, eligibility and call behavior can use outdated local data instead of current Dataverse state, causing incorrect block/allow decisions or dialing outdated numbers.
Useful? React with 👍 / 👎.
| except (ValueError, OSError) as exc: | ||
| # Malformed transport fixture / missing file — surfaces as exit_status=failed. |
There was a problem hiding this comment.
The runner only catches QueueItemNotFound, ValueError, and OSError; Dataverse write failures from the adapter can propagate out of process_one_queue_item uncaught. In those cases run-crm exits via an unstructured exception path, and terminal progress/failure status is not normalized into the CLI’s exit-status contract for operators.
Useful? React with 👍 / 👎.
| artifact_path = out or Path(slice2_config.dataverse.mapping_artifact) | ||
| try: | ||
| scaffold = load_mapping(artifact_path) |
There was a problem hiding this comment.
discover-crm documents --out as an output override, but the implementation also uses that path as the input scaffold. If --out points to a new file, discovery fails before running because load_mapping reads the non-existent output path instead of the configured mapping artifact.
Useful? React with 👍 / 👎.
| .get("value", []) | ||
| ) | ||
| if existing: | ||
| return str(existing[0].get(f"{entity}id")) |
There was a problem hiding this comment.
On an idempotency pre-query hit, the adapter derives the Dataverse record ID using f"{entity}id" instead of the mapped entity primary key. For entities whose primary key doesn’t follow that naming pattern, correlations are recorded with a missing/incorrect dataverse_record_id, degrading resume/audit fidelity for repeated runs.
Useful? React with 👍 / 👎.
| try: | ||
| queue_item = loader.load(selector) | ||
| except MappingError as exc: | ||
| return CrmRunReport(exit_status="failed", message=f"queue loader mapping error: {exc}") |
There was a problem hiding this comment.
DataverseQueueLoader.load can raise queue-mapping failures (for example, an unmapped option-set value), but the runner only catches MappingError around the load call. Those loader errors currently escape uncaught, so run-crm can crash instead of returning a structured failed report with operator-visible messaging.
Useful? React with 👍 / 👎.
1. Owner bind respects entity-set (Copilot #1): _resolve_task_owner now returns (owner_id, entity_set) and _owner_bind targets /teams(<id>) when the resolved owner verified as a team. A team override would otherwise POST under /systemusers and 400 against real Dataverse. 2. Active-enabled gate + default validation (Copilot #2): systemuser lookup now filters by isdisabled=false (teams have no such column). The configured default owner is verified too; if neither override nor default resolves to an active enabled user/team, Task emission is blocked with an operator-visible task_owner_default_unverifiable warning rather than writing an unverified id (FR-025 — never write an unverified owner). 3. Simplified --write flag (Copilot #3): dropped the misleading --dry-run alias on run-crm. The CLI now declares only --write and the help/error message explicitly point to US2 / T024-T026 for the dry-run wiring. Tests: - Contract test seeds the default owners as active enabled systemusers and adds: disabled-systemuser override falls back, team override binds to /teams, and default-owner-unverifiable blocks emission. - Integration test seeds both default owners as active enabled systemusers so the US1 happy/alternate/blocked paths verify under the stricter gate. 271 / 273 pass (28 in this PR's new suite; the 2 remaining failures are the pre-existing constitution-sync regressions inherited from fab3ebd). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b9291fa3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| ) | ||
| translator = MappingTranslator(mapping) | ||
| report = verify(client, mapping, now_utc_ms=clk.now_utc_ms()) |
There was a problem hiding this comment.
verify(client, mapping, ...) performs live Dataverse calls and can raise DataverseError (for example auth failures, 5xx, or transport timeouts), but this call is outside the runner’s exception handling. In those startup-failure scenarios, run_one_crm_item throws instead of returning a CrmRunReport, so run-crm bypasses its documented exit-status/report contract and terminates with an unstructured exception path.
Useful? React with 👍 / 👎.
| return None | ||
| if queue_item_id: | ||
| return ExplicitId(queue_item_id) | ||
| return NextReady(campaign or "") |
There was a problem hiding this comment.
When --next-ready is used without --campaign, _build_selector always constructs NextReady("") instead of using the configured [run].campaign value; this contradicts the CLI contract that --campaign is an override and drops the default campaign scope. As a result, next-ready selection is built without the intended campaign constraint and can target the wrong queue population.
Useful? React with 👍 / 👎.
| conversation = ( | ||
| _load_conversation_fixture(conversation_fixture) if conversation_fixture else None | ||
| ) |
There was a problem hiding this comment.
Conversation fixture parsing happens before the try block that maps ValueError/OSError to exit_status="failed". If the fixture file is missing or malformed, _load_conversation_fixture(...) raises directly and run-crm exits through an unhandled exception instead of the documented operator-visible failed report/exit-status path.
Useful? React with 👍 / 👎.
| # ----- public protocol surface ----------------------------------------- | ||
|
|
||
| def emit_phone_call_activity(self, payload: PhoneCallActivityPayload) -> None: | ||
| entity = self._t.entity_logical_name("phone_call_activity") |
There was a problem hiding this comment.
The adapter uses mapping logical_name values directly as Web API resource paths (for example phonecall, task, medx_callqueueitem), but Dataverse record operations must target the table EntitySetName (e.g., phonecalls, tasks, systemusers). This causes 404/OData path errors against real Dataverse even when metadata verification succeeds, so write-enabled runs can fail before creating or patching any CRM records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/opencloser/cli.py:412
run-crmbuilds the QueueSelector before loadingslice2.toml, so--next-readyfalls back toNextReady(campaign or ""). This drops the configured[run].campaigndefault and can issue an unscoped/empty campaign selector despite FR-032 requiring a campaign selector. Loadslice2_configfirst (or pass it into_build_selector) so--next-readyuses--campaignwhen provided, otherwise defaults toslice2_config.run.campaign, and error if the resulting campaign is empty.
selector = _build_selector(
queue_item_id=queue_item_id,
next_ready=next_ready,
campaign=campaign,
)
if selector is None:
typer.echo(
"error: provide exactly one of --queue-item-id or --next-ready",
err=True,
)
raise typer.Exit(code=2)
try:
slice1_config = load_config(config_path)
slice2_config = load_slice2_config(slice2_config_path)
except (FileNotFoundError, ValueError) as exc:
typer.echo(f"error: config load failed: {exc}", err=True)
raise typer.Exit(code=2) from None
try:
secrets = load_dataverse_secrets()
except Slice2ConfigError as exc:
typer.echo(f"error: {exc}", err=True)
raise typer.Exit(code=2) from None
clock = SystemClock()
token_provider = DataverseTokenProvider(secrets)
client = DataverseClient(secrets.env_url, token_provider, slice2_config.retry)
conn = store.connect(slice1_config.state.db)
try:
report = run_one_crm_item(
selector=selector,
transport_fixture=transport_fixture,
conversation_fixture=conversation_fixture,
slice1_config=slice1_config,
slice2_config=slice2_config,
client=client,
conn=conn,
clock=clock,
)
finally:
conn.close()
client.close()
token_provider.close()
typer.echo(f"exit_status: {report.exit_status}")
if report.session_id:
typer.echo(f"session_id: {report.session_id}")
if report.final_disposition:
typer.echo(f"final_disposition: {report.final_disposition.value}")
if report.queue_item_id:
typer.echo(f"queue_item_id: {report.queue_item_id}")
if report.artifact_dir:
typer.echo(f"artifact_dir: {report.artifact_dir}")
if report.warnings:
typer.echo("warnings:")
for w in report.warnings:
typer.echo(f" {w.code}: {w.message}")
if report.message:
typer.echo(f"message: {report.message}")
raise typer.Exit(code=_EXIT_CODE.get(report.exit_status, 2))
def _build_selector(
*,
queue_item_id: str | None,
next_ready: bool,
campaign: str | None,
) -> QueueSelector | None:
if bool(queue_item_id) == bool(next_ready):
return None
if queue_item_id:
return ExplicitId(queue_item_id)
return NextReady(campaign or "")
| return str(existing[0].get(f"{entity}id")) | ||
| response = self._client.post(entity, json=body) | ||
| entity_uri = response.headers.get("OData-EntityId", "") | ||
| return _parse_record_id_from_uri(entity_uri) |
| rows = ( | ||
| self._client.get( | ||
| entity, | ||
| params={ | ||
| "$filter": f"{self._t.mapping.entities['queue_item'].primary_id or f'{entity}id'} " | ||
| f"eq {queue_item_id}", |
| entity = self._t.entity_logical_name("queue_item") | ||
| primary_id = self._t.mapping.entities["queue_item"].primary_id or f"{entity}id" | ||
| rows = ( | ||
| self._client.get( | ||
| entity, | ||
| params={ | ||
| "$filter": f"{primary_id} eq {queue_item_id}", | ||
| "$select": override_field, | ||
| "$top": "1", | ||
| }, |
| for entity, entity_set, extra_filter in ( | ||
| ("systemuser", "systemusers", " and isdisabled eq false"), | ||
| ("team", "teams", ""), | ||
| ): | ||
| try: | ||
| rows = ( | ||
| self._client.get( | ||
| entity, | ||
| params={ | ||
| "$filter": f"{entity}id eq {owner_id}{extra_filter}", | ||
| "$select": f"{entity}id", | ||
| "$top": "1", | ||
| }, |
| # ultimately the queue-status transition_reason via the adapter, but never changes | ||
| # the exit status. |
P1 — Use Dataverse EntitySetName for record API paths (Codex). The
adapter now hits /phonecalls, /tasks, /medx_callqueueitems, /systemusers,
/teams via a `_entity_set_name` helper (logical_name + "s" with override
hooks for irregular plurals). Previously hitting /phonecall etc. would
404 against real Dataverse even when metadata verification succeeded.
P1 — Drop scheduledend free-form-text write (Codex). preferred_callback_window
is intentionally free-form ("Thursday afternoon"); writing to Dataverse
`scheduledend` (Edm.DateTimeOffset) 400s on non-ISO input. The window
now surfaces in the task description only.
P1 — Normalize verify() failures into structured CrmRunReport (Codex).
`run_one_crm_item` catches `DataverseError`/`MetadataError` from the
readiness `verify()` call and returns exit_status=blocked instead of
bubbling an unhandled exception to the CLI.
P1 — Validate non-empty OData-EntityId after POST (Copilot). An empty
header would otherwise lead to a CONFIRMED crm_correlations row with no
record id, masking the failure on every later resume/audit. Now raises
DataverseWriteBackError.
P2 — Use mapped primary_id instead of f"{entity}id" (Sourcery + Codex).
The adapter exposes a `_primary_id` helper that prefers the mapping's
explicit `entities[...].primary_id` and falls back to the singular-form
derivation only when the mapping omits it — important for activity tables
whose primary key is `activityid`, not `<logical>id`.
P2 — Catch queue-loader failures + Dataverse adapter exceptions in the
run coordinator (Codex). `run-crm` now produces operator-visible
exit_status=failed reports for QueueLoadError/MappingError/DataverseError
/DataverseWriteBackError/missing fixture files instead of unhandled
exceptions.
P2 — Refresh ALL mutable queue_item fields when staging from Dataverse
(Codex). _stage_queue_item now refreshes phone_number/timezone/
attempt_count in addition to callable_status/dnc_flag, so eligibility
decisions are made against current Dataverse state rather than stale
locally-cached data. State DAO `update_queue_item_status` extended
additively with the new optional kwargs.
P2 — discover-crm: load scaffold from configured artifact, not --out
(Codex). `--out` is now strictly an output override; the scaffold is
always read from slice2.toml's mapping_artifact, so `discover-crm
--out new-path.json` works on the first run.
P2 — `--next-ready` falls back to the configured [run].campaign (Codex).
A missing `--campaign` no longer drops the campaign scope; the CLI
loads slice2.toml first and uses `slice2_config.run.campaign` as the
default.
P2 — Active-enabled check (Codex + Copilot reflected). systemuser
verification now filters by `isdisabled eq false`; a disabled user is
treated as unverifiable. The default-owner verification stays — neither
override nor default verifying blocks Task emission.
Cleanup — Remove unused ReadinessError (Sourcery); update the runner
doc comment that incorrectly described non-E.164 warnings as being
written to transition_reason (they go to queue.last_error via the
adapter's _compose_last_error).
Test fixture changes — The in-process Dataverse fake now auto-aliases
each registered entity with its `<name>s` form so the foundation
queue_loader (logical names) and the adapter (entity-set names) hit the
same data through one fake instance. `_handle_create`/`_handle_patch`
derive the singular-form primary id so both URL forms resolve. Contract
+ integration tests now query fake.created/fake.patched via the
entity-set names the adapter actually uses.
271 / 273 pass — only the pre-existing constitution-sync regressions
inherited from fab3ebd remain.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 538fd0711c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ref.primary_id | ||
| # Strip the trailing 's' from the entity-set form to recover the logical | ||
| # name when the mapping omits primary_id. | ||
| return entity_set.rstrip("s") + "id" |
There was a problem hiding this comment.
When an entity mapping omits primary_id, _primary_id falls back to entity_set.rstrip("s") + "id", which produces incorrect keys for Dataverse activities (for example phonecallid instead of activityid). _idempotent_create uses that value in $select, so phone-call idempotency pre-queries can fail with a 400 before any write is attempted, breaking write-enabled runs for mappings that do not manually populate activity primary IDs.
Useful? React with 👍 / 👎.
| if phone_number is not None: | ||
| fields.append("phone_number = ?") | ||
| values.append(phone_number) |
There was a problem hiding this comment.
This update path treats None as “do not update”, so when Dataverse clears a nullable field (like phone or timezone), _stage_queue_item cannot propagate that null and leaves stale local values in place. On repeat runs, eligibility can be evaluated against outdated phone/timezone data instead of the current CRM row, causing incorrect allow/block outcomes.
Useful? React with 👍 / 👎.
| mapping = load_mapping(slice2_config.dataverse.mapping_artifact) | ||
| except MappingError as exc: |
There was a problem hiding this comment.
run_one_crm_item only catches MappingError around load_mapping, but load_mapping also surfaces schema-invalid artifacts via Pydantic ValidationError. In that case the exception escapes this function instead of returning a structured CrmRunReport, so run-crm can terminate with an unhandled traceback rather than the documented operator-visible status.
Useful? React with 👍 / 👎.
| except PermanentDataverseError as exc: | ||
| self._mark_progress( | ||
| payload.session_id, | ||
| queue_status_update_done=False, | ||
| last_error=str(exc), | ||
| run_status=RunStatus.BLOCKED, | ||
| ) |
There was a problem hiding this comment.
On permanent Dataverse PATCH failures, this branch updates writeback_progress and re-raises, but never writes a crm_correlations row with write_status=failed. That loses per-record failure state for audit/recovery flows, making it impossible to distinguish “not attempted” from “attempted and failed” for queue-status writes.
Useful? React with 👍 / 👎.
| `systemuser`/`team`/`account`). Irregular plurals (e.g. `opportunity` → | ||
| `opportunities`) require an entry in `_ENTITY_SET_OVERRIDES`. | ||
| """ | ||
| return _ENTITY_SET_OVERRIDES.get(logical_name, logical_name + "s") |
There was a problem hiding this comment.
The adapter derives Web API collection paths by appending "s" to logical names, but Dataverse explicitly allows EntitySetName to differ from logical collection naming. For custom tables with non-default or irregular entity set names, this generates incorrect URLs and causes 404/route failures at runtime even when metadata verification passes.
Useful? React with 👍 / 👎.
| scaffold = load_mapping(scaffold_path) | ||
| except MappingError as exc: |
There was a problem hiding this comment.
The discovery command only catches MappingError around load_mapping, so a syntactically valid but schema-invalid mapping artifact (raised by Pydantic validation) escapes uncaught and crashes the command. This bypasses the CLI’s normal error: output and exit-code handling for operator-facing discovery failures.
Useful? React with 👍 / 👎.
| record_id = self._idempotent_create( | ||
| entity_key=entity_key, | ||
| idempotency_field=idempotency_field, | ||
| idempotency_value=payload.session_id, | ||
| body=self._phone_call_body(payload, idempotency_field), |
There was a problem hiding this comment.
If _idempotent_create fails during phone-call creation, the exception propagates before any correlation or progress row is written. That leaves no writeback_progress/crm_correlations failure marker for this step, so recovery and audit logic can’t tell which write failed mid-run (the same gap exists for task creation via the same helper path).
Useful? React with 👍 / 👎.
P1 — Wrap Pydantic ValidationError as MappingError in `load_mapping`.
Both the runner and `discover-crm` now see a single error type for
"mapping artifact is wrong"; previously a schema-invalid JSON file
escaped as an unhandled traceback.
P1 — Require explicit `entities[...].primary_id`. The previous
fallback derived `<entity_set>-rstrip('s')+"id"`, which silently
produces `phonecallid` where Dataverse activity tables expect
`activityid`. The adapter now raises `MappingError` if the mapping
omits primary_id, pointing the operator at `dataverse_mapping.json`.
The test fixture mapping populates primary_id explicitly for queue
items, phone calls (phonecallid), and tasks (activityid).
P1 — Add explicit `entity_set_name` to DataverseEntityRef. Real
Dataverse exposes `EntityDefinition.EntitySetName` which can differ
from `logical_name + "s"` for irregular plurals; the adapter prefers
the mapping's value and falls back to a curated override map +
`+s` only when the field is unset. The override map now covers the
common irregular plurals (`opportunity`, `businessunit`, etc.).
P1 — Record failed `crm_correlations` rows on POST/PATCH errors.
`emit_phone_call_activity` / `emit_queue_status_update` / `emit_task`
each now persist a `write_status=FAILED` correlation + stamp the
resume ledger with `run_status=blocked` and `last_error` before
re-raising. Audit/recovery flows can now distinguish "not attempted"
from "attempted and failed" per CRM record kind.
P2 — Null-preserving queue refresh. `_stage_queue_item` now uses a
new `store.replace_queue_item_mutable_fields` DAO that overwrites
every mutable column from the live Dataverse snapshot — null clears
included. The old `update_queue_item_status` retains its
"`None` means leave it alone" partial-update semantics for callers
that want that behavior; the docstring documents the split.
Test fixture changes: the in-process Dataverse fake now stamps
`activityid` as an alias on activity-table creates (phonecall, task,
email, appointment) so a mapping that addresses activities by the
canonical `activityid` primary key resolves correctly. Activity test
attribute sets include `activityid` so $select / $filter pass through
the fake's strict known-field check.
271 / 273 pass; pre-existing constitution-sync failures inherited
from fab3ebd remain.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| return None | ||
| if queue_item_id: | ||
| return ExplicitId(queue_item_id) | ||
| return NextReady(campaign or "") |
| key. Prefers the mapping's `entity_set_name` when populated (discover-crm | ||
| reads `EntityDefinition.EntitySetName`); falls back to the irregular-plural | ||
| override map / `+s` rule otherwise.""" |
| Real-environment discovery should populate this from | ||
| `EntityDefinition.EntitySetName` so irregular plurals work without code | ||
| changes (FR-001/FR-004). |
| def replace_queue_item_mutable_fields( | ||
| conn: sqlite3.Connection, | ||
| queue_item: QueueItem, | ||
| ) -> None: | ||
| """Overwrite every mutable column on the local queue row from a fresh source | ||
| snapshot — including null clears. | ||
|
|
||
| `update_queue_item_status` treats `None` as "don't update", so a Dataverse | ||
| field that has been cleared to null since the last run never propagates to | ||
| local state. Slice 2's runner needs the opposite semantics when re-staging | ||
| a row read live from Dataverse: every mutable column should mirror the | ||
| snapshot, null included, so eligibility evaluates against current state | ||
| rather than whatever a prior run last wrote. | ||
| """ |
`--next-ready` now requires a non-empty campaign — the CLI checks the effective campaign (`--campaign` or `slice2.toml [run].campaign`) and exits with an actionable error when both are blank. This stops the loader from silently picking items outside the operator's intended campaign while the loader-side filter remains a queue-loader (T014) follow-up tracked in US3. Docstrings on `_derive_entity_set` and `DataverseEntityRef` now match the actual `discover-crm` behavior — it refreshes `_meta` only and does NOT auto-populate `entity_set_name` from `EntityDefinition.EntitySetName`. Operators set the field by hand on PR review; auto-population is tracked as a follow-on enhancement. New `replace_queue_item_mutable_fields` DAO is now covered by three unit tests: full overwrite, null-clearing of nullable columns, and FK-child preservation (sessions remain pinned to the refreshed queue row). These pin the new "always overwrite, null included" semantics that the runner depends on for accurate eligibility decisions. 274 / 276 pass; pre-existing constitution-sync failures from fab3ebd remain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673a59aa4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return None | ||
| if queue_item_id: | ||
| return ExplicitId(queue_item_id) | ||
| return NextReady(campaign or "") |
There was a problem hiding this comment.
In the --next-ready path this builds NextReady(campaign), but the selector’s campaign is never enforced downstream, so the run can pick any ready row regardless of the requested/configured campaign. When multiple campaigns share the same queue table, run-crm --next-ready can process the wrong campaign’s item and write back to the wrong record set.
Useful? React with 👍 / 👎.
| message=str(exc), | ||
| warnings=adapter.warnings(), | ||
| ) | ||
| except (DataverseError, DataverseWriteBackError) as exc: |
There was a problem hiding this comment.
The orchestration try/except only normalizes DataverseError and DataverseWriteBackError, but adapter emit paths can also raise MappingError (for example when a required primary_id or option-set mapping is missing), so run-crm can still terminate with an unhandled traceback instead of returning a structured exit_status="failed" report. Fresh evidence: the adapter now explicitly re-raises MappingError from emit paths, while this handler does not include it.
Useful? React with 👍 / 👎.
P1 — Apply the campaign filter in queue_loader's NextReady path (contracts/dataverse-queue-loader.md). When the mapping carries a `queue.campaign` field the loader scopes the query to the selector's campaign. When the mapping omits the field — the current Slice 2 fixture — the loader stays campaign-agnostic and the CLI gate (`--next-ready` requires a non-empty campaign) is the user-facing signal. Two unit tests pin the new behavior: campaign scoping when mapped, campaign-agnostic when unmapped. P2 — Catch MappingError from adapter emit paths in the runner's write-back try/except. The adapter's `_primary_id` raises MappingError when the mapping omits `entities[...].primary_id`; the runner now normalizes that into an operator-visible exit_status=failed report alongside DataverseError/DataverseWriteBackError. 276 / 278 pass; pre-existing constitution-sync failures from fab3ebd remain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| # selector carries the scope the contract demands (cli-slice2.md). Without | ||
| # one, the loader's query is currently campaign-agnostic — surfacing this | ||
| # at the CLI is the smallest fix that avoids silently picking items | ||
| # outside the operator's intended campaign. (Filtering the actual loader | ||
| # query by campaign is a queue-loader (T014) concern and tracked in US3.) |
| key. Prefers the mapping's `entity_set_name` when populated (discover-crm | ||
| reads `EntityDefinition.EntitySetName`); falls back to the irregular-plural |
| clauses = [f"{status_field} eq {ready_value}"] | ||
| campaign_field_ref = self._t.mapping.fields.get("queue.campaign") | ||
| if campaign_field_ref is not None and selector.campaign: | ||
| campaign_field = campaign_field_ref.logical_name | ||
| clauses.append(f"{campaign_field} eq '{selector.campaign}'") |
| entity_set = self._entity_set(entity_key) | ||
| primary_id = self._primary_id(entity_key) | ||
| existing = ( | ||
| self._client.get( | ||
| entity_set, | ||
| params={ | ||
| "$filter": f"{idempotency_field} eq '{idempotency_value}'", | ||
| "$select": primary_id, | ||
| "$top": "1", |
P1/security — Escape single quotes in OData `$filter` string literals. A new `errors.odata_string_literal(value)` helper wraps a value in single quotes with embedded `'` doubled per OData v4 spec (section 5.1.1.6.1). Used by: - queue_loader.py — `campaign` filter on the NextReady path - adapter.py — idempotency pre-query on every emit_* Without this escape, a campaign like `O'Brien` would terminate the literal and invalidate the whole filter clause; the helper also shuts the door on OData-filter injection for user-supplied values. Three unit tests pin the escape semantics: basic wrap, embedded single-quote doubling, empty string. Docstrings — Two more stale "discover-crm reads EntitySetName" / "queue-loader is campaign-agnostic" remnants corrected. The `_entity_set` docstring now matches reality (`entity_set_name` set by hand on PR review today), and the cli.py comment notes the loader DOES filter by campaign when the mapping carries `queue.campaign`. 279 / 281 pass; pre-existing constitution-sync failures from fab3ebd remain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
openCloser/src/opencloser/crm/dataverse/queue_loader.py
Lines 55 to 59 in 78af75a
Query queue rows via entity set names, not logical names
DataverseQueueLoader.load builds record queries using entity_logical_name("queue_item"), which yields the table logical name (for example medx_callqueueitem) rather than the Web API entity-set path (medx_callqueueitems). Dataverse record endpoints are keyed by entity set names, so this path can return 404 in production environments and make run-crm fail during queue intake instead of loading the selected row.
openCloser/src/opencloser/crm/dataverse/queue_loader.py
Lines 132 to 136 in 78af75a
Resolve facility lookups against the target entity set path
Facility-name hydration calls GET on lookup_target directly, but mapping lookup targets are logical names (e.g., account), while Dataverse data queries require entity-set paths (e.g., accounts). With a standard mapping this call can 404, which bubbles out of queue loading and turns otherwise valid runs into queue loader error failures.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
P1 — queue_loader uses entity-set names for record queries. The `DataverseQueueLoader.load` path was sending OData record GETs to the table's logical name (e.g. `/medx_callqueueitem`); real Dataverse keys record endpoints by the entity-set name (`/medx_callqueueitems`) and would 404 the request before the row could be loaded. P1 — Facility-name lookup now derives the entity-set form for the mapping's `lookup_target` too. `lookup_target` is a Dataverse logical- name reference (e.g. `account`), so a record query needs the entity-set form (`accounts`); the previous code 404'd against real environments on every queue intake that needed facility hydration. Refactor — Centralized entity-set resolution in `mapping.py` as `resolve_entity_set(mapping, entity_key)` (prefers `entity_set_name`, falls back to `derive_entity_set(logical)`). The override map for known-irregular plurals (`activitypointers`, `opportunities`, `businessunits`, `transactioncurrencies`) moved with it. Adapter + queue_loader now share one source of truth; the duplicated `_derive_entity_set` / `_ENTITY_SET_OVERRIDES` in `adapter.py` are gone. 279 / 281 pass; pre-existing constitution-sync failures from fab3ebd remain. The in-process Dataverse fake's `<name>` ↔ `<name>s` alias keeps the new entity-set queries resolving against the same records the foundation tests register by logical name. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ccc2f7198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if resolved is None: | ||
| # FR-025 unverifiable-default branch already recorded an | ||
| # operator-visible warning; nothing to write to the ledger. | ||
| return |
There was a problem hiding this comment.
When _resolve_task_owner(...) returns None (for example, default owner missing/disabled), emit_task exits early instead of surfacing an error. That means task-producing dispositions can complete without creating a Dataverse Task, yet the runner still reports a successful run based on disposition, so operators can miss required human follow-up work. This path should propagate a failure/blocked status (or record failed task write state) instead of silently returning.
Useful? React with 👍 / 👎.
| "activity": "activitypointers", | ||
| "opportunity": "opportunities", | ||
| "businessunit": "businessunits", | ||
| "currency": "transactioncurrencies", |
There was a problem hiding this comment.
The irregular-entity-set override is keyed as "currency", but Dataverse logical names use transactioncurrency. As written, derive_entity_set("transactioncurrency") falls through to the default pluralizer and produces transactioncurrencys, which yields incorrect Web API paths whenever this fallback is used without an explicit entity_set_name mapping.
Useful? React with 👍 / 👎.
| except PermanentDataverseError: | ||
| continue |
There was a problem hiding this comment.
_owner_entity_set swallows every PermanentDataverseError and treats it like “owner not found”. In environments with auth/permission regressions (e.g., 401/403 from systemusers/teams reads), task-owner verification silently degrades to “unverifiable”, which then causes Task emission to be skipped/fallback rather than failing the run as a Dataverse error. This can hide production misconfiguration and produce completed runs without required task write-back.
Useful? React with 👍 / 👎.
| campaign_field_ref = self._t.mapping.fields.get("queue.campaign") | ||
| if campaign_field_ref is not None and selector.campaign: | ||
| campaign_field = campaign_field_ref.logical_name | ||
| clauses.append(f"{campaign_field} eq {odata_string_literal(selector.campaign)}") |
There was a problem hiding this comment.
The --next-ready query only adds a campaign predicate when queue.campaign is mapped; otherwise it runs campaign-agnostic selection even though a campaign is required at the CLI. Fresh evidence: this new branch explicitly keeps the query unscoped when queue.campaign is absent, so multi-campaign environments can process the wrong queue row and write back to the wrong campaign.
Useful? React with 👍 / 👎.
P1 (Codex) — emit_task now SURFACES unverifiable-owner as a failed
task write rather than silently returning. The earlier behavior let a
disposition that requires a Task (interested_callback_requested,
needs_human_review, etc.) complete the run as `completed` while the
required Task never wrote, hiding broken human-follow-up handoff.
When `_resolve_task_owner` returns None the adapter now raises
`DataverseWriteBackError`; the outer try/except records a failed task
correlation + stamps `writeback_progress(run_status=blocked)`, and the
runner normalizes that into `exit_status=failed`.
P1 (Codex) — `_owner_entity_set` no longer swallows every
PermanentDataverseError. Only HTTP 404 (the table genuinely absent
in this environment) falls through to the next entity set; 401/403
permission regressions and 400 malformed queries propagate so
emit_task records a failed task correlation rather than silently
degrading to "owner unverifiable → fallback".
P1 (Codex) — queue_loader rejects NextReady when `queue.campaign`
is missing from the mapping. In a multi-campaign environment a
campaign-agnostic query would pick the wrong row; failing fast at
queue-load with a clear `QueueLoadError` is the safer default.
Existing tests updated to seed the queue.campaign mapping where
campaign-scoped selection is expected.
Copilot — `_record_failure` no longer downgrades a prior CONFIRMED
correlation to FAILED, and no longer flips `*_done` from True to
False. A later transient failure on an already-confirmed record (e.g.
a duplicate emit retry) preserves the confirmed state and the
completed `*_done` flag; only when the prior state was unset/not-done
does the failure path mark them.
P2 (Codex) — `_ENTITY_SET_OVERRIDES` had the wrong logical-name key
(`"currency"` instead of `"transactioncurrency"`). With that key,
`derive_entity_set("transactioncurrency")` fell through to the
`+s` rule and produced `transactioncurrencys` — invalid Web API
path. Fixed.
Contract test updated: `test_task_blocked_when_default_owner_unverifiable`
now expects `DataverseWriteBackError` AND verifies the failed
`crm_correlations` row.
281 / 283 pass; pre-existing constitution-sync failures from fab3ebd
remain.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 280692b968
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except (MappingError, QueueLoadError, DataverseError) as exc: | ||
| return CrmRunReport(exit_status="failed", message=f"queue loader error: {exc}") |
There was a problem hiding this comment.
DataverseQueueLoader.load() can surface Pydantic validation failures from QueueItem(...) construction (for example, a negative attempt_count or other schema-invalid row data), but this handler only normalizes MappingError, QueueLoadError, and DataverseError. In those data-corruption cases run-crm exits via an unhandled exception instead of returning a structured exit_status="failed" report for operators.
Useful? React with 👍 / 👎.
P2 — Catch Pydantic `ValidationError` from queue_loader.load() in the run coordinator. `DataverseQueueLoader.load(...)` constructs a `QueueItem` from the Dataverse row, and a schema-corrupted row (negative `attempt_count`, etc.) surfaces as `pydantic.ValidationError` from that constructor. The runner's loader try/except only caught `MappingError`/`QueueLoadError`/`DataverseError`, so a corrupted row escaped as an unhandled traceback instead of producing the documented operator-visible `exit_status=failed` report. 281 / 283 pass; pre-existing constitution-sync failures from fab3ebd remain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| try: | ||
| slice2_config = load_slice2_config(slice2_config_path) | ||
| except (FileNotFoundError, ValueError) as exc: | ||
| typer.echo(f"error: could not load {slice2_config_path}: {exc}", err=True) | ||
| raise typer.Exit(code=2) from None |
| try: | ||
| slice1_config = load_config(config_path) | ||
| slice2_config = load_slice2_config(slice2_config_path) | ||
| except (FileNotFoundError, ValueError) as exc: | ||
| typer.echo(f"error: config load failed: {exc}", err=True) | ||
| raise typer.Exit(code=2) from None |
| # loader's NextReady path now filters by campaign when the mapping carries | ||
| # a `queue.campaign` field; when the mapping omits that field, the query | ||
| # falls back to a campaign-agnostic match and this CLI gate is the only | ||
| # user-facing protection against picking the wrong campaign's row. |
| # Don't regress a `*_done=True` flag set by a prior successful emit — | ||
| # only mark it False when it hasn't been completed yet. | ||
| progress = self._load_progress(session_id) | ||
| already_done = bool(progress and getattr(progress, progress_key)) | ||
| progress_update: dict[str, bool] = {} if already_done else {progress_key: False} | ||
| self._mark_progress( | ||
| session_id, | ||
| last_error=str(error), | ||
| run_status=RunStatus.BLOCKED, |
| "failed", | ||
| ] | ||
|
|
||
| # Anchored E.164: leading +, then 1-15 digits. |
| warnings=adapter.warnings(), | ||
| ) | ||
|
|
||
| # 7) Stamp terminal progress for the resume ledger. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962eb93c5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._record_failure( | ||
| session_id=payload.session_id, | ||
| record_kind=CrmRecordKind.PHONE_CALL_ACTIVITY, | ||
| error=exc, | ||
| progress_key="phone_call_activity_done", | ||
| ) | ||
| raise |
There was a problem hiding this comment.
This handler calls _record_failure(...) and then re-raises, but emit_phone_call_activity is executed inside with store.transaction(conn) in core/orchestrator.py (_run_allowed_session and _finalize_blocked). When the exception propagates, that outer transaction rolls back, which also rolls back the failure rows just written on the same connection (crm_correlations/writeback_progress). In production write failures therefore still lose the durable failure marker needed for resume/audit, even though _record_failure is invoked; the failure write needs to be committed outside that transaction boundary (or through a savepoint/independent connection).
Useful? React with 👍 / 👎.
| except (DataverseError, MappingError) as exc: | ||
| self._record_failure( | ||
| session_id=payload.session_id, | ||
| record_kind=CrmRecordKind.QUEUE_STATUS, | ||
| error=exc, | ||
| progress_key="queue_status_update_done", | ||
| dataverse_record_id=payload.queue_item_id, | ||
| ) | ||
| raise |
There was a problem hiding this comment.
The queue-status path records failure state in _record_failure(...) and immediately re-raises, but this call still runs under the orchestrator’s with store.transaction(conn) block around crm.emit_queue_status_update(...). Fresh evidence: that outer transaction rollback will undo the just-written crm_correlations/writeback_progress rows on the same connection, so a rejected queue PATCH still leaves no durable failed marker for resume/audit despite the new failure handler.
Useful? React with 👍 / 👎.
| except (DataverseError, DataverseWriteBackError, MappingError) as exc: | ||
| self._record_failure( | ||
| session_id=payload.session_id, | ||
| record_kind=CrmRecordKind.TASK, | ||
| error=exc, | ||
| progress_key="task_done", | ||
| ) | ||
| raise |
There was a problem hiding this comment.
emit_task now records failure metadata and re-raises on Dataverse or mapping errors, but it is called inside the orchestrator’s with store.transaction(conn) block for task emission. If task creation fails, that outer transaction rollback also reverts the _record_failure writes on this same SQLite connection, so sessions that fail during task write-back still lose the crm_correlations/writeback_progress failure marker needed for FR-023 resume and audit.
Useful? React with 👍 / 👎.
P1 (Codex×3) — Persist `_record_failure` writes outside the
orchestrator's rolling-back transaction. The orchestrator wraps every
`crm.emit_*` in `with store.transaction(conn)`; when an emit raises,
the ROLLBACK undoes ANY rows the adapter wrote — including the
crm_correlations(failed) / writeback_progress(blocked) markers the
new failure ledger depends on. The adapter now accepts an optional
`failure_conn_factory` callable and the runner wires it to a peer
SQLite connection on the same WAL db. Each failure write commits
independently of the rolling-back transaction so resume/audit can
distinguish "not attempted" from "attempted and failed" in production.
Copilot — Catch `pydantic.ValidationError` in `discover-crm` and
`run-crm` CLI config loads. `load_config` / `load_slice2_config` raise
ValidationError (not ValueError) on schema-invalid TOML; the previous
handlers crashed with a traceback. Both commands now exit 2 with a
clean error message.
Copilot — Preserve a terminal `COMPLETED` run_status in
`_record_failure`. The same pattern as the CONFIRMED-correlation
preservation: a later transient failure on an already-completed
session must not regress the ledger from COMPLETED back to BLOCKED.
Copilot — Fix stale cli.py comment that claimed the queue-loader
falls back campaign-agnostic when `queue.campaign` is unmapped. The
loader now raises `QueueLoadError` in that case; the CLI gate is
just an earlier-exit convenience for missing operator input.
Copilot — Fix the E.164 regex comment. `^\+[1-9]\d{1,14}$` is
2-15 digits (mandatory non-zero leading digit), not 1-15.
Copilot — Fix step numbering in `run_one_crm_item` after the earlier
refactor renumbered the steps but missed updating the trailing
comment (5 → 6, was incorrectly 7).
281 / 283 pass; pre-existing constitution-sync failures from fab3ebd
remain.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea64270356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| failure_conn = self._failure_conn_factory() | ||
| try: | ||
| store.upsert_crm_correlation(failure_conn, correlation) | ||
| store.upsert_writeback_progress(failure_conn, new_progress) |
There was a problem hiding this comment.
_record_failure
When an emit fails, this path writes failure rows through a new SQLite connection while the orchestrator is still inside with store.transaction(conn) around crm.emit_* (which already performed a write via idempotency tracking). SQLite permits only one writer at a time, so these peer upserts can hit OperationalError: database is locked, masking the original Dataverse failure and dropping the intended crm_correlations/writeback_progress failure markers. This needs a strategy that does not write from a second connection before the outer transaction releases its write lock.
Useful? React with 👍 / 👎.
| def _open_failure_conn() -> sqlite3.Connection: | ||
| peer = store.connect(state_db_path) | ||
| # `store.connect()` configures WAL + foreign_keys; combined with |
There was a problem hiding this comment.
The fallback writer is opened from slice1_config.state.db instead of the already-open conn passed into run_one_crm_item. If those diverge (for example embedded callers passing an injected connection, or :memory: configurations), failure recording runs against a different database (or a fresh in-memory DB without schema), which can lose failure markers or raise no such table while handling a Dataverse error.
Useful? React with 👍 / 👎.
P1 — Replace the peer-connection failure-ledger fix with an in-memory staging + deferred-write model. SQLite is single-writer, so the round-12 "open a second connection during emit_*" approach would deadlock (or hit SQLITE_BUSY) because the orchestrator's `with store.transaction(conn)` block already holds the write lock while the emit is in flight. The peer connection writes never reached disk in production environments — masking the original Dataverse failure with an OperationalError on top. New shape: - `_record_failure` (called inside the failing emit) appends to a new `self._pending_failures: list[_PendingFailure]` instead of touching the DB. The orchestrator's rollback fires without erasing anything. - The runner's `except (DataverseError, DataverseWriteBackError, MappingError)` handler now calls `adapter.flush_pending_failures()` AFTER the exception bubbles up (the rollback has released the write lock). Each staged failure is persisted via the shared connection with the same preserve-CONFIRMED / preserve-`*_done` / preserve-COMPLETED semantics from round-12. - The `failure_conn_factory` constructor parameter is gone — no more peer connections, no more lock contention. Contract test `test_task_blocked_when_default_owner_unverifiable` updated: calls `adapter.flush_pending_failures()` explicitly to verify the failed `crm_correlations` row that downstream resume/audit will see (the test bypasses the runner, so it has to drive flush itself, just as the runner does in production). 281 / 283 pass; pre-existing constitution-sync failures from fab3ebd remain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| def _mark_progress( | ||
| self, | ||
| session_id: str, | ||
| *, | ||
| phone_call_activity_done: bool | None = None, | ||
| queue_status_update_done: bool | None = None, | ||
| task_done: bool | None = None, | ||
| last_error: str | None = None, | ||
| run_status: RunStatus = RunStatus.IN_PROGRESS, | ||
| ) -> None: |
Preserve terminal `run_status` in `_mark_progress`. The default of
`RunStatus.IN_PROGRESS` was unconditionally overwriting the column on
every per-emit `*_done` flip, so a follow-up call (idempotent retry,
mid-run progress update) could regress an already-terminal `COMPLETED`
or `BLOCKED` ledger row back to `IN_PROGRESS`. The default is now
`None` ("leave the existing value alone"); the first call for a
brand-new session still gets `IN_PROGRESS` via `_new_progress`, and
explicit terminal transitions (the runner's `finalize_progress` /
`flush_pending_failures`) pass the value they want.
281 / 283 pass; pre-existing constitution-sync failures from fab3ebd
remain.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0fce54aee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| account_logical = account_field.lookup_target or "account" | ||
| account_entity_set = derive_entity_set(account_logical) |
There was a problem hiding this comment.
Use resolve_entity_set (or equivalent mapping lookup) here instead of derive_entity_set. The commit adds entities[*].entity_set_name overrides for irregular/custom tables, but _facility_name still hard-derives the collection from lookup_target; when a deployment sets a non-default entity set for the facility lookup target, this builds the wrong Web API path and queue loading fails with a 404 despite a valid mapping artifact.
Useful? React with 👍 / 👎.
| base = entity[:-1] if entity.endswith("s") else entity | ||
| return f"{base}id" |
There was a problem hiding this comment.
This helper derives primary keys by stripping one trailing s, which breaks irregular entity-set names (for example opportunities -> opportunitieid, transactioncurrencies -> transactioncurrencieid). In those cases the fake stores/looks up rows under the wrong key, so PATCH/lookup behavior diverges from Dataverse and integration tests can fail or pass incorrectly for mappings that use explicit irregular entity_set_name values.
Useful? React with 👍 / 👎.
| artifact_path.write_text( | ||
| json.dumps(body, sort_keys=True, indent=2, ensure_ascii=False) + "\n", | ||
| encoding="utf-8", | ||
| ) |
There was a problem hiding this comment.
discover-crm writes the refreshed mapping file without handling filesystem errors, so paths like --out in a non-existent directory (or permission-denied targets) raise an unhandled OSError traceback instead of returning the command’s structured error: message and exit code 2. This breaks operator-facing failure behavior for a common invocation path where users override the output location.
Useful? React with 👍 / 👎.
Adapt PR #6 to the current Slice 2 umbrella APIs: keep the umbrella Dataverse config/model/fake contracts, preserve PR #6 write-enabled adapter and runner behavior, and update the US1/adapter tests for entity-set aliases, owner lookup aliases, and activity primary IDs.\n\nVerification:\n- uv run ruff check .\n- uv run pytest tests/unit/test_dataverse_metadata_queue.py -q --no-cov\n- uv run pytest tests/contract/test_dataverse_adapter_contract.py -q --no-cov\n- uv run pytest tests/integration/test_us1_write_enabled.py -q --no-cov\n- uv run pytest tests/integration/test_cli.py -q --no-cov\n- uv run pytest -q --no-cov (406 passed, 2 known constitution-sync failures)
|
@codex review |
| def _fetch_queue_last_session(self, queue_item_id: str) -> str | None: | ||
| entity_set = self._entity_set("queue_item") | ||
| primary_id = self._primary_id("queue_item") | ||
| last_session_field = self._t.logical_name("queue.last_session_id") | ||
| rows = ( | ||
| self._client.get( | ||
| entity_set, | ||
| params={ | ||
| "$filter": f"{primary_id} eq {queue_item_id}", | ||
| "$select": last_session_field, |
| def _lookup_owner_override(self, queue_item_id: str) -> str | None: | ||
| override_field = self._t.mapping.task_owner_override_field | ||
| if not override_field: | ||
| return None | ||
| entity_set = self._entity_set("queue_item") | ||
| primary_id = self._primary_id("queue_item") | ||
| rows = ( | ||
| self._client.get( | ||
| entity_set, | ||
| params={ | ||
| "$filter": f"{primary_id} eq {queue_item_id}", | ||
| "$select": override_field, | ||
| "$top": "1", | ||
| }, |
| for entity_set, primary_id, extra_filter in ( | ||
| ("systemusers", "systemuserid", " and isdisabled eq false"), | ||
| ("teams", "teamid", ""), | ||
| ): | ||
| try: | ||
| rows = ( | ||
| self._client.get( | ||
| entity_set, | ||
| params={ | ||
| "$filter": f"{primary_id} eq {owner_id}{extra_filter}", | ||
| "$select": primary_id, | ||
| "$top": "1", | ||
| }, | ||
| ) |
| def _mapping_with_campaign() -> DataverseMapping: | ||
| """`_MAPPING` augmented with a `queue.campaign` field — required by the | ||
| loader's NextReady path for campaign-scoped selection.""" | ||
| mapping = _MAPPING.model_copy(deep=True) | ||
| mapping.fields["queue.campaign"] = type(next(iter(_MAPPING.fields.values())))( | ||
| entity="queue_item", | ||
| logical_name="medx_campaign", | ||
| type="string", | ||
| ) | ||
| return mapping |
| _MAPPING_FIXTURE = _REPO_ROOT / "tests/fixtures/dataverse/dataverse_mapping.json" | ||
|
|
||
| _T = "2026-05-22T16:00:00.000Z" | ||
| _QID = "q-contract-0001" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7020250a95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value = rows[0].get(override_field) | ||
| if value in (None, ""): | ||
| return None |
There was a problem hiding this comment.
_value field
This code reads the override from rows[0].get(override_field), but Dataverse lookup/owner columns are exposed as computed _<logical>_value properties, not the bare logical name. When task_owner_override_field points to a lookup (the normal owner/team case), this returns None, so valid overrides are silently ignored and every Task falls back to the default owner, breaking FR-025 behavior in real Dataverse environments.
Useful? React with 👍 / 👎.
| queue_item.queue_item_id, | ||
| conn=conn, | ||
| config=slice1_config, | ||
| eligibility=BuiltinEligibilityEvaluator(), | ||
| transport=FixtureDrivenTransport(transport_dir), |
There was a problem hiding this comment.
This call does not pass a redaction_layer, so process_one_queue_item falls back to the default redaction policy/retention instead of using slice2_config.redaction. As a result, operator-configured settings (for example retention = "summary-only" or custom regex patterns) are ignored in run-crm, which can write transcripts with the wrong retention behavior and violate the run’s configured privacy policy.
Useful? React with 👍 / 👎.
| report = verify(client, mapping, now_utc_ms=clk.now_utc_ms()) | ||
| except (DataverseError, MetadataError) as exc: |
There was a problem hiding this comment.
Readiness currently validates mapping metadata only, then proceeds to run, but FR-025 defines approved owner overrides/defaults in terms of active Dataverse users/teams verified up front. With an invalid default owner or override target, the run can place a call and perform earlier writes before failing later in emit_task, leaving a partial write-back instead of a clean startup block. Add owner/team verification to the pre-run readiness gate so misconfiguration is caught before any CRM mutation.
Useful? React with 👍 / 👎.
|
@codex review |
|
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Two new Phase 9 polish tasks address the genuine task-level gaps surfaced by the post-checklist /speckit-checklist reverification pass (commit 10b5c30): - T049 (CHK046-CHK052): document the run-report schema in contracts/cli-slice2.md — required fields per spec §Constitution Alignment §Auditability, the artifact format, and the dry-run vs write-enabled field sets. Makes observability requirements first-class. - T050 (CHK068-CHK069): document the write-back progress state machine in data-model.md §1 and the exit-status mapping in contracts/cli-slice2.md — the four resume states with mutually exclusive, exhaustive criteria, allowed transitions, and triggering events. The remaining reverification.md items either validate requirement quality of the existing scope (no new task needed) or are already covered by existing tasks (T017-T048). Re-running /speckit.tasks wholesale would risk renumbering T035-T039 which are referenced in merged PR #4/#5/#6 commits; surgical adds preserve that history. §Task Summary totals updated: 49 -> 51 (Polish 6 -> 8); 28 unique IDs parallel marker count 26 -> 28. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
/#5/#6 + cross-validated MEDIUMs) Closes 3 HIGH findings from the 2026-05-24 swarm code-review pass plus 2 cross-validated MEDIUMs: HIGH #4 (conflict-detection + dataverse-adapter): TOCTOU race between the conflict-check GET and the queue-status PATCH was the exact failure mode T045 was supposed to close. Now closed via Dataverse `If-Match` / `@odata.etag` optimistic concurrency: the baseline captures the row's etag at load time; emit_queue_status_update sends `If-Match: <etag>` on the PATCH; 412 Precondition Failed → CrmConflictError(["@odata.etag"]). HIGH #5 (resume-coordinator + conflict-detection): `_snapshot_resume_baseline` silently swallowed MappingError + TransientDataverseError, bypassing the CHK061 conflict-stop on resume. Now: errors propagate; only "row not found" (`loaded is None`) is a silent skip. Transient/permanent classification is done by the outer resume_session except → RESUME_NEEDED vs BLOCKED. HIGH #6 (conflict-detection): `last_session_id` was not in the baseline, so a concurrent session writing to that column between our load and our PATCH would clobber it undetected. Now captured in QueueBaseline and compared in _check_conflict; mismatch → CrmConflictError. MEDIUM (conflict-detection + dataverse-adapter — cross-validated): status_value None-vs-0 asymmetry. Baseline coerced missing→0; current GET returned None; produced spurious conflicts AND missed real ones. Fixed by typing QueueBaseline.status_value as int|None and dropping the coercion. MEDIUM (dataverse-adapter): deleted row mid-run produced exit_status="failed" instead of "blocked" + conflict_detected. Now `_check_conflict` raises CrmConflictError(["__row_deleted__"]) on missing row. Implementation: - models.py: QueueBaseline.status_value → int|None; new last_session_id + etag fields; preserve_values typed dict[str, Any] (was object). - queue_loader.py: _baseline_from_row captures last_session_id + @odata.etag; raw status (no coercion). - adapter.py: _check_conflict reads last_session_field, compares status raw, raises on deleted-row; emit_queue_status_update sends If-Match header and maps 412 → CrmConflictError. - client.py: DataverseClient.patch accepts optional headers kwarg. - resume.py: _snapshot_resume_baseline re-raises errors; only None skips. - fake.py: emit `@odata.etag` per row on GET (monotonic version counter); honor If-Match on PATCH (412 on mismatch); bump version on every PATCH. Tests added (5 in test_us4_idempotency.py): - test_t045_conflict_on_human_adding_preserve_field_value — None→non-None - test_t045_conflict_on_last_session_id_change — concurrent-session clobber - test_t045_deleted_row_yields_conflict_not_failed — row-deletion semantics - test_t045_if_match_412_raises_conflict_error — TOCTOU/ETag race closed - test_resume_snapshot_failure_re_raises_to_blocked — snapshot 401 → blocked Result: 616 pass (+5 new Pass 1B tests) / 2 pre-existing constitution_sync failures unrelated. ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
All three are factually-wrong package docstrings / comments left over from Slice 2's early-scaffold phase; the underlying code shipped many commits ago but the surrounding prose was never refreshed. 1. `src/opencloser/redaction/__init__.py` — the docstring claimed the package was a "boundary scaffold only" with `RedactionLayer` landing in a later sub-PR (#4). `layer.py` is present and exercised by the `RedactionLayer.from_config` readiness path in `slice2/runner.py`. Updated to describe what ships. 2. `src/opencloser/slice2/__init__.py` — same shape: claimed runner / resume-coordinator land in later sub-PRs (#5, #6). Both `runner.py` and `resume.py` exist and are the surface modules the CLI imports. Rewrote to list the surface modules and their roles. 3. `src/opencloser/core/orchestrator.py:158` — comment promised that `transport.pre_validate_fixture()` raises `MalformedFixtureError`, but the hook can also raise plain `ValueError` from `_resolve_fixture_path` (e.g. path-traversal / invalid fixture_id). Reworded to state both shapes — the operator-visible guarantee (`no session row, no eligibility-decision row, no attempt consumed, no Dataverse queue change` per SC-006) is unchanged. No behavior change. Imports + US5 fixture-validation tests pass. ruff clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
End-to-end write-enabled loop against the Dataverse fake. Stacks on PR #3.
Summary by Sourcery
Add Dataverse-backed write-enabled CRM loop with CLI commands and runner for Slice 2 and cover it with contract and integration tests.
New Features:
Enhancements:
Tests: