Skip to content

fix(history): keep accepted outputs reported when retention fails - #105

Merged
mberrys merged 3 commits into
devfrom
cc/code-review-ebc857
Sep 28, 2026
Merged

mberrys merged 3 commits into
devfrom
cc/code-review-ebc857

Conversation

@mberrys

@mberrys mberrys commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

What changed

Follow-up review fixes for 846e869 (#685 retention enforcement + #664 GWG claim). In #685, if history retention failed after a rollback or repair had already been accepted and published, the workflow stopped early and the result was hidden:

  • Editor rollback (EditorHost::requestFixRollback): the rolled-back revision was written but never opened, and the recorded-revisions list was not refreshed. It now still opens and refreshes. The retention error is announced with the restore message, and the function returns false.
  • PdfTool repair: retention ran before --report-file was written and before outputs were registered, so a failure dropped the report. Retention now runs after the report, output registration, and console output. It still exits ProcessingFailure with history.retention-failed.
  • Preflight coverage_scope.claim: the #664 rewording dropped "Loop does not claim formal GWG conformance." for every profile family. That sentence is restored, and the sheetfed-offset/packaging certificate limit is kept.

Targets dev (integration); it reaches stable only through promotion dev → unstable → stable after the unstable checks pass.

Release changelog

Topic fragment: changes/cc-code-review-ebc857.md (Category: fixed).

Proof

  • python scripts/agent/check-change.py --base origin/stable reports fail: every build:* target and focused_tests fail because this worktree has no configured build directory (configuring one needs approval), and clang_tidy is incomplete (no compile_commands.json/clang-tidy). changelog, source_integrity, architecture_catalog, policy_adapters, preflight_truth_source, qml_mirror_parity, search_budget_gate, independent_validation_gate, qt_test_runtime, architecture_contracts, and format:* for all three touched files pass. Not built or tested locally; relying on CI.
  • One changes/<sanitized-head-branch>.md fragment added (Category, Audience, Breaking-Change, Summary), plus changes/cc-code-review-ebc857.evidence.yaml
  • Changed behaviour has a test that fails without the change: no. The retention-failure paths have no tests yet (see open findings).
  • Protected-path or contract change named above: none. The claim text changes, but coverage_scope keeps the same shape, and no fixture contains the #664 string.

Internal logic (touched behavior-bearing code)

  • Guard clauses handle invalid, stale, cancelled, absent, unauthorized, and terminal cases before the happy path
  • Untrusted input is parsed once at the boundary into trusted typed or domain state, with no repeated checks downstream
  • Invalid state stops before partial mutation or publication and returns a descriptive error or result: retention failure still returns failure, but only after the already-published output is reported
  • Names carry the domain intent, and comments explain rationale rather than restating the code

Anti-slop pass

  • Redundant or explanatory comments that do not match the file's style removed
  • Abnormal defensive checks and broad try/catch blocks removed where a trusted upstream boundary already guarantees the invariant, with real boundary and safety checks kept
  • No any or equivalent cast added only to suppress a type error
  • Python imports stay at file scope unless a local import is required
  • Generated boilerplate, needless wrappers, and local-style drift removed
  • Validation, security, cancellation, provenance, and failure handling preserved

Anti-slop summary (1-3 sentences):

Only two one-line rationale comments were added, both explaining why retention runs after publication. Failure handling was kept: both paths still return failure and name the retention error.

Security and rollback

  • Untrusted input validated at the trust boundary; no new unsafe construct without an inline justification
  • Rollback: revert the two commits; no schema, persistence, or fixture changes.

Docs

  • None needed: docs/OPERATION_HISTORY.md already states that a retention failure is reported after the accepted event and output are preserved, and this PR makes the code do that.

Open review findings (not addressed here)

  • appendPreflightAuditRun points are the only unprotected (approval None) rollback points and never trigger retention; the four #685 call sites only create approved (protected) points, so default-policy retention never evicts anything.
  • PDFOperationHistoryStore::enforceRetention deletes artifact files before COMMIT; a later failure leaves non-evicted points whose artifacts are gone.
  • Action List and add-bleed report retention failure as history.write-failed and skip output registration.
  • Repair's report file says passed when retention later fails. Recording retention in the report would change the report schema.
  • The editor retention warning is only an accessibility announcement; there is no visible status surface.

Self-review (BSP-002 §4.3)

  • Reviewed in the diff view, not the editor, at least 30 minutes after the final commit; overnight if the change touches security-sensitive code, data handling, or public API surface

🤖 Generated with Claude Code

mberrys and others added 3 commits September 23, 2026 14:10
A retention failure after an accepted rollback or repair no longer hides
the published result: the editor still opens the restored revision and
refreshes its rollback points, and repair writes its report and registers
its outputs before enforcing retention. The per-run preflight coverage
claim again states that Loop makes no formal GWG conformance claim for
every profile family, alongside the sheetfed-offset/packaging limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mberrys
mberrys changed the base branch from stable to dev September 28, 2026 00:03
@mberrys
mberrys merged commit c6c8f9d into dev Sep 28, 2026
19 checks passed
@mberrys
mberrys deleted the cc/code-review-ebc857 branch September 30, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant