feat: record recurring failures and gate their repair (#1557) - #1586
Conversation
Add failure signatures, append-only recurrence observations, runtime ledger orchestration, persistence, statistics, API contract updates, and the related documentation and tests. Keep recurrence evidence write-only on the outcome path and expose the required ScopeStats.recurrence block. Constraint: ScopeStats.recurrence is a required breaking API field; Windows-only baseline failures and seven pre-existing ty diagnostics remain outside this change. Tested: 109 focused tests; 48 contract tests; 26 integration-manifest tests; ruff check and format check; generated schema and enabled server OpenAPI match. Not-tested: make check remains non-zero only because of the seven baseline ty diagnostics; full unit test comparison is 2252 passed, 58 skipped, 47 failed on the implementation tree versus 2177 passed, 58 skipped, 46 failed on clean baseline. Co-authored-by: OmX <omx@oh-my-codex.dev> Signed-off-by: Xin.Zh <alexstocks@foxmail.com>
Ignore Mermaid diagrams in license-eye because the checker cannot determine their comment style, and update the existing base-access E2E assertion for the optional Experience failure field. Constraint: Preserve the required ScopeStats.recurrence and ExperienceProposal.failure contract without weakening validation. Tested: targeted base-access E2E test and API contract/JS tests pass; Ruff check and format check pass; licenserc parses as YAML. Co-authored-by: OmX <omx@oh-my-codex.dev> Signed-off-by: Xin.Zh <alexstocks@foxmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect evidence integrity, concurrency, runtime provenance, Review routing, and statistics.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Implements evidence-bound recurring-failure tracking with append-only persistence, runtime recording, Review routing, statistics, API updates, tests, and documentation.
Changes:
- Adds failure metadata, recurrence matching, and verdict logic.
- Adds runtime ledger recording and persistence tables.
- Updates statistics, OpenAPI models, generated schemas, and mappings.
- Adds tests and bilingual operational documentation.
File summaries
| File | Reviewed change |
|---|---|
tests/test_cli.py |
Updates CLI statistics fixtures. |
tests/test_api_contract.py |
Verifies recurrence API contracts. |
tests/e2e/test_recurring_failure_repair.py |
Adds cross-component recurrence scenarios. |
tests/builtin/runtime/test_statistics.py |
Tests recurrence statistics. |
tests/builtin/runtime/test_recurrence_ledger.py |
Tests runtime ledger recording. |
tests/builtin/persistence/test_recurrence_repository.py |
Tests recurrence persistence. |
tests/builtin/artifacts/experience/test_search.py |
Tests failure search rendering. |
tests/builtin/artifacts/experience/test_recurrence.py |
Tests recurrence matching and verdicts. |
tests/builtin/artifacts/experience/test_models.py |
Tests failure model validation. |
src/powercontext/server/mapping.py |
Maps recurrence API values. |
src/powercontext/http/_generated/schema.py |
Updates generated schemas. |
src/powercontext/http/_generated/models.py |
Updates generated API models. |
src/powercontext/http/__init__.py |
Exports transport models. |
src/powercontext/builtin/statistics/models.py |
Defines recurrence statistics models. |
src/powercontext/builtin/statistics/aggregation.py |
Aggregates per-scope recurrence data. |
src/powercontext/builtin/statistics/__init__.py |
Exports statistics types. |
src/powercontext/builtin/runtime/statistics.py |
Loads recurrence statistics. |
src/powercontext/builtin/runtime/relational.py |
Integrates recurrence and Review routing. |
src/powercontext/builtin/runtime/recurrence.py |
Orchestrates runtime recurrence recording. |
src/powercontext/builtin/persistence/tables.py |
Defines recurrence tables. |
src/powercontext/builtin/persistence/recurrence.py |
Persists append-only recurrence events. |
src/powercontext/builtin/persistence/__init__.py |
Exports recurrence persistence. |
src/powercontext/builtin/artifacts/experience/search.py |
Renders Experience failure data. |
src/powercontext/builtin/artifacts/experience/recurrence.py |
Implements recurrence matching and evidence rules. |
src/powercontext/builtin/artifacts/experience/prompts.py |
Updates Experience generation guidance. |
src/powercontext/builtin/artifacts/experience/models.py |
Defines structured failure metadata. |
src/powercontext/builtin/artifacts/experience/__init__.py |
Exports failure types. |
src/powercontext/artifacts/models.py |
Adds immutable artifact values. |
openapi/powercontext.yaml |
Extends the API contract. |
docs/zh/prd/1557_recurring_failure_repair_prd.md |
Adds the Chinese PRD. |
docs/zh/docs/operate/troubleshoot.md |
Updates Chinese troubleshooting guidance. |
docs/zh/design/1557_recurring_failure_repair_sequence-diagram.mermaid |
Adds the recurrence sequence diagram. |
docs/zh/design/1557_recurring_failure_repair_class-diagram.mermaid |
Adds the recurrence class diagram. |
docs/en/docs/operate/troubleshoot.md |
Updates English troubleshooting guidance. |
Review details
Suppressed comments (8)
src/powercontext/builtin/artifacts/experience/recurrence.py:190
- A Handoff may cite the same immutable Experience revision more than once. This comprehension preserves duplicate refs, so one matching revision is counted twice and
match_resultbecomesambiguous, even thoughfreeze_candidate_sethas correctly deduplicated the snapshot; that suppresses the expectedrecurredevent. Deduplicate eligible refs by immutable ref identity before counting them.
return tuple(
ref
for ref, content in sorted(candidates, key=lambda pair: _ref_identity(pair[0]))
if _stored_signature_key(content) == key
)
src/powercontext/builtin/artifacts/experience/recurrence.py:279
- The ledger columns and generated digests use the canonical 71-character form
sha256:<64 lowercase hex>, but this validator accepts any string with only the prefix. Values such assha256:not-a-digestcan therefore survive model validation and be persisted as locators, weakening the immutable evidence identity; validate the exact length and lowercase hexadecimal body here.
if not self.item_digest.startswith(_DIGEST_PREFIX):
raise ValueError("item_digest must be a sha256 digest") # noqa: TRY003
src/powercontext/builtin/artifacts/experience/search.py:45
- Prepared context calls this renderer, but the new failure block only emits the recall cue and symptom. The
repair_surfaceand both verification bindings are therefore stored and exposed by the API but omitted from the context the agent receives, so a recalled failure cannot route the repair or know which condition/check to exercise. Include those fields in the rendered block.
if content.failure is not None:
lines.append(f"Failure cue: {content.failure.signature.recall_cue}")
if content.failure.signature.symptom is not None:
lines.append(f"Symptom: {content.failure.signature.symptom}")
src/powercontext/builtin/persistence/recurrence.py:141
- This has the same silent-failure problem for observation writes: any integrity violation is returned as
False, so a malformed event can be mistaken for an already-recorded event and the cursor can advance without the ledger row. Restrict the no-op result to a verified duplicate of the relevant uniqueness key and re-raise unrelated violations.
try:
await connection.execute(insert(RECURRENCE_OBSERVATION_TABLE).values(**values))
except IntegrityError:
return False
src/powercontext/builtin/runtime/recurrence.py:123
link is Noneconflates an outcome with no receipt and one whosehandoff_receipt_refis present but cannot be resolved. The latter is a provenance gap, but this branch snapshots scope heads and recordsrecurred, turning missing linkage into apparent recall failure. Only a genuinely receipt-less outcome should usescope_heads; unresolved receipt windows should remain unclassified or be surfaced as coverage gaps.
candidates: _Contents = contents if link is not None else await self._experience_heads(connection)
src/powercontext/builtin/runtime/recurrence.py:413
- Handoff citations are not required to be unique, so this slice can pass duplicate revisions into
_contents. A single Experience cited twice then appears as two eligible candidates and is classified asambiguous; enough duplicates can also consume the 64-entry bound before later distinct revisions are considered. Deduplicate by exact ArtifactRef identity before applying the limit.
citations = handoff_experience_citations(handoff.content)[:MAX_RECURRENCE_CANDIDATES]
return await self._contents(connection, citations)
src/powercontext/builtin/runtime/relational.py:1743
- This proposal targets the exact revision frozen in the match. If a newer revision is published while a Handoff still cites the older one,
_validate_targetrejects this candidate because only the current head may be targeted; the exception occurs in the same transaction asrecord_window, rolling back the append-only recurrence rows and cursor. Stale revisions need to be skipped/deferred or explicitly retargeted so Review proposal failure cannot erase the ledger event.
reason=proposal.reason,
src/powercontext/builtin/runtime/statistics.py:259
- If one active Handoff contains a missing or stale Experience citation,
get_many()raises and this handler returns an empty tuple, dropping every other valid citation from the coverage calculation. A scope with one valid unlinked citation and one missing citation will therefore report zero gaps; resolve citations independently or use a partial batch lookup and retain successful results.
try:
experiences = await self._artifacts.get_many(connection, self._scope_id, tuple(cited))
except RepositoryNotFoundError:
return ()
- Files reviewed: 37/37 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Teingi
left a comment
There was a problem hiding this comment.
Reviewed 4ee2d2ae. All 146 focused tests passed, but separate SQLite probes reproduced these three issues and the three existing findings I replied to. The probes cover the public runtime and ledger behavior; they do not include a real LLM or OceanBase run.
Teingi
left a comment
There was a problem hiding this comment.
Rechecked 5f51b7f. I recommend addressing the P1 and three P2 findings before merging. The signature-specific false positive is detailed in my reply to the existing discussion; the other three findings are inline below.
The 210 focused tests passed on this exact head, and I reran all four counterexamples using public SQLite runtime/repository probes and a disposable local MySQL 8.0.45 instance. The third recurrence now reaches Review successfully. These probes do not include a real OceanBase or external LLM run.
Lore: A scope-head match must use every active Experience head because its persisted candidate snapshot is evidence for an immutable recurrence decision. Constraint: Keep the independent 64-entry statistics Handoff scan unchanged; this only removes undocumented ledger candidate truncation. Tested: 78 focused recurrence tests and 48 API contract tests pass; Ruff, targeted ty, and git diff checks pass. Not-tested: Full Windows prek remains blocked by nine pre-existing artifact-processing and os.WNOHANG ty diagnostics outside this change. Co-authored-by: OmX <omx@oh-my-codex.dev>
|
针对
关于 当前 Head 的 quality、Python 3.11-3.14、SQLite/OceanBase acceptance 及其余可见检查均已通过。该结论仅覆盖当前 CI 和已处理评论,不替代所需的人工 review 决策。 |
Lore: Refresh the recurring failure repair PR on top of the latest master while keeping the generated HTTP API surface derived from the merged OpenAPI contract. Constraint: Resolve only the generated model conflict; keep recurrence behavior and the new generic Artifact API both intact. Tested: make contract-test; uv run pytest -q tests/builtin/artifacts/experience/test_recurrence.py tests/builtin/persistence/test_recurrence_repository.py tests/builtin/review/test_service.py tests/builtin/runtime/test_recurrence_ledger.py tests/builtin/runtime/test_statistics.py tests/e2e/test_recurring_failure_repair.py; uv run pytest -q tests/e2e/test_topic_memory_generic_api.py; uv run ruff check src/powercontext/http/_generated/models.py src/powercontext/http/_generated/schema.py tests/test_api_contract.py; git diff --cached --check. Co-authored-by: OmX <omx@oh-my-codex.dev>
|
@Teingi current head |
Teingi
left a comment
There was a problem hiding this comment.
Reviewed df8e9392. I recommend fixing the P1 and P2 below before merging.
All 218 focused tests passed. Separate SQLite probes reproduced both findings, including a valid-evidence retry case with one injected transient Review failure. I also confirmed the column-width fix on MySQL 8.0.45 in strict mode and the candidate-snapshot fix beyond 64 entries. Normal third-recurrence Review admission succeeds. This validation does not include a real OceanBase or external LLM run.
| async with connection.begin_nested(): | ||
| await connection.execute(insert(RECURRENCE_MATCH_TABLE).values(**values)) |
There was a problem hiding this comment.
[P1] Keep recurrence savepoints inside the outer SQLite transaction
With the current SQLite profile, if the window has only read before this call, there is no actual BEGIN before SAVEPOINT. Releasing the savepoint commits the ledger write, so a later failure cannot roll it back. The same applies to append_observation().
On this head, both in-memory and file SQLite retain one match and one event after an outer rollback; an explicit-BEGIN control retains neither. Through the public runtime, I used valid evidence and injected one transient error before the third recurrence's Review insert. After the error, recurred=3 was already persisted. Retrying advanced the cursor but returned candidate_count=0 with an empty Inbox: _record_recurrences() treats the retained event as a replay and skips the proposal.
Please ensure these savepoints participate in a real outer transaction, and cover rollback/retry of the ledger, candidate, and cursor together.
There was a problem hiding this comment.
Fixed in 774a0de7.
append_match() and append_observation() now ensure SQLite has a real outer BEGIN before opening their nested savepoints. This keeps the ledger writes inside the incubation transaction even when the window only performed reads before the append. Added rollback regressions for both match and observation writes after read-only work; both now disappear after the outer rollback.
Local validation:
33 passedfortests/builtin/persistence/test_recurrence_repository.py,tests/builtin/runtime/test_recurrence_ledger.py, andtests/e2e/test_recurring_failure_repair.py.- ruff check, ruff format --check, git diff --check, and scoped
ty checkpassed on the changed files. - Full
ty checkstill reports pre-existing unrelated diagnostics inartifact_processing.py,opencode.py, andtest_artifact_processing.py; none point at this change.
GitHub checks are running on 774a0de7.
| item = _item(outcome, failure_ref) | ||
| if item is None: | ||
| continue |
There was a problem hiding this comment.
[P2] Resolve nested evidence before recording recurrence verdicts
The admission fix validates nested citations when proposing a failure Experience, but this ledger path still checks only the Outcome item and its digest. It never resolves the item's evidence. _record_avoided() has the same gap.
On this head, I first approved a failure Experience using valid evidence, then used public Source capture to ingest failed checks citing SourceRef(source_type='content', source_id='never-existed'). Two incubation windows still produced recurred=1 and recurred=2. In another scope, a condition and passing check with that nonexistent citation produced avoided=1 and reset an existing streak from 1 to 0.
Please resolve the relevant nested Source, Artifact, and Memory references in the current scope before freezing matches or recording terminal verdicts. Unresolvable evidence must remain insufficient; rejecting a candidate only when the third recurrence reaches Review leaves the earlier ledger entries incorrect.
There was a problem hiding this comment.
Fixed in 774a0de7.
The recurrence ledger now validates the nested evidence on the exact TaskOutcome items before recording terminal verdicts. _freeze_matches() validates the failure item evidence before freezing a match that can become recurred; _record_avoided() validates both the condition and passed-check evidence before writing avoided. Unresolvable Source, Artifact, or Memory citations now leave that item insufficient instead of writing an incorrect ledger verdict.
Added regressions for both sides: a failed check citing a missing Source no longer records recurred, and a passed condition/check pair citing a missing Source no longer records avoided.
Local validation:
33 passedfortests/builtin/persistence/test_recurrence_repository.py,tests/builtin/runtime/test_recurrence_ledger.py, andtests/e2e/test_recurring_failure_repair.py.- ruff check, ruff format --check, git diff --check, and scoped
ty checkpassed on the changed files. - Full
ty checkstill reports pre-existing unrelated diagnostics inartifact_processing.py,opencode.py, andtest_artifact_processing.py; none point at this change.
GitHub checks are running on 774a0de7.
Lore: SQLite savepoints can commit independently when no real outer BEGIN exists, and recurrence verdicts must be backed by resolvable nested evidence. Constraint: Ledger append savepoints must roll back with the incubation transaction, and terminal recurred/avoided verdicts must not be written when their TaskOutcome item evidence cannot be resolved in the current scope. Tested: uv run pytest -q tests/builtin/persistence/test_recurrence_repository.py tests/builtin/runtime/test_recurrence_ledger.py tests/e2e/test_recurring_failure_repair.py Tested: uv run ruff check src/powercontext/builtin/persistence/recurrence.py src/powercontext/builtin/runtime/recurrence.py src/powercontext/builtin/runtime/relational.py tests/builtin/persistence/test_recurrence_repository.py tests/builtin/runtime/test_recurrence_ledger.py tests/e2e/test_recurring_failure_repair.py Tested: uv run ruff format --check src/powercontext/builtin/persistence/recurrence.py src/powercontext/builtin/runtime/recurrence.py src/powercontext/builtin/runtime/relational.py tests/builtin/persistence/test_recurrence_repository.py tests/builtin/runtime/test_recurrence_ledger.py tests/e2e/test_recurring_failure_repair.py Tested: uv run ty check src/powercontext/builtin/persistence/recurrence.py src/powercontext/builtin/runtime/recurrence.py src/powercontext/builtin/runtime/relational.py tests/builtin/persistence/test_recurrence_repository.py tests/builtin/runtime/test_recurrence_ledger.py tests/e2e/test_recurring_failure_repair.py Co-authored-by: OmX <omx@oh-my-codex.dev>
Lore: Resolve PR oceanbase#1596 conflicts after PR oceanbase#1586 landed on master. Constraint: Keep both recall-gate ExperienceSearchOutcome exports and recurrence failure model exports/imports. Tested: uv run pytest -q tests/builtin/review/test_service.py Tested: uv run pytest -q tests/builtin/runtime/test_recall_sufficiency.py tests/e2e/test_recall_sufficiency_gate.py tests/builtin/artifacts/experience/test_recurrence.py tests/builtin/runtime/test_recurrence_ledger.py Tested: uv run ruff check src/powercontext/builtin/artifacts/experience/__init__.py tests/builtin/review/test_service.py Tested: uv run ruff format --check src/powercontext/builtin/artifacts/experience/__init__.py tests/builtin/review/test_service.py Tested: git diff --check Not-tested: Full tox and live external LLM runs were not rerun for this merge-conflict-only update. Co-authored-by: OmX <omx@oh-my-codex.dev>
Closes #1554
Implements RFC #1557 (recurring failure repair) with an append-only recurrence ledger and evidence-bound runtime recording.
What changes are included
ExperienceContent.failurewith stable failure signatures, repair surfaces, and verification criteria.selected,recurred,avoided, andunknownonly from positive evidence; read/prepare paths do not write recurrence events.ScopeStats.recurrenceaggregation and API/OpenAPI generated models, mapping, and schema updates.Compatibility
ScopeStats.recurrenceis required in the public API. This is a breaking response-schema change; consumers must accept the new recurrence block. The recurrence tables are append-only and do not add a new artifact family or MCP tool.Validation
109 passed.48 passed.26 passed.ruff check tests/ src/: passed.ruff format --check tests/ src/: passed.openapi/powercontext.yamlexactly.2252 passed, 58 skipped, 47 failed; clean baseline2177 passed, 58 skipped, 46 failed. The extra implementation-tree failure was the stale CLI stats fixture and is fixed in this PR; the remaining failures are existing Windows/platform/environment failures in the same groups as baseline.make checkintegration and prek hooks pass;ty checkremains non-zero with 7 pre-existing diagnostics in multiprocessing stubs and Windows-onlyos.WNOHANGtests.AI usage statement
Implemented and validated with OpenAI Codex (GPT-5-based coding agent). The agent generated and edited code, tests, OpenAPI generated artifacts, and documentation; all changes were reviewed against the repository contract and verified with the commands listed above.