Skip to content

fix(sync): preserve policy progress during optional upload failures - #2958

Closed
kantorcodes wants to merge 5 commits into
hgp/batch-08-policy-proof-prerequisitesfrom
hgp/batch-08-policy-telemetry-isolation
Closed

kantorcodes wants to merge 5 commits into
hgp/batch-08-policy-proof-prerequisitesfrom
hgp/batch-08-policy-telemetry-isolation

Conversation

@kantorcodes

@kantorcodes kantorcodes commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Keep policy updates and receipt progress available when optional uploads are delayed. Preserve authorization failures, completed upload counts, and clear status for each affected upload.

Validation: 78 focused tests pass, including malformed responses, recovery, and status rendering. Lint, formatting, and independent deterministic report checks pass.

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • release/2.2
  • release/3.1

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Advanced

Run ID: 895da38c-416b-401b-84c3-5560c81347e7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Preserve policy progress during telemetry outages

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Preserves accepted policies, acknowledgements, and upload cursors when optional telemetry fails.
• Separates telemetry degradation from receipt, validation, and policy application outcomes.
• Adds stable failure reasons, retry coverage, CLI status, and operator documentation.
Diagram

graph TD
  A["Sync runner"] -->|persists policy| B[("Policy store")] -->|saved progress| C["Telemetry coordinator"] --> D["Pain upload"] --> F["Sync summary"] --> G["CLI renderer"]
  C --> E["Event upload"] --> F
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Durable telemetry job queue
  • ➕ Fully decouples policy synchronization latency from telemetry transport availability
  • ➕ Enables independent retry schedules and backoff policies
  • ➖ Requires queue lifecycle, persistence, and daemon changes
  • ➖ Introduces substantially more operational complexity than the current cursor-based retries
2. Typed outcomes from every uploader
  • ➕ Avoids normalizing a mixture of exceptions and returned failure summaries
  • ➕ Makes authorization and degradation states explicit at uploader boundaries
  • ➖ Requires broader changes to established pain-signal and guard-event APIs
  • ➖ Creates more compatibility risk for callers outside this synchronization path

Recommendation: Keep the PR's centralized telemetry coordinator and existing durable cursors. It provides the required isolation without migrations or wire-contract changes, while explicitly preserving authorization failures; a job queue would be disproportionate, and fully typed uploader outcomes can be considered in a broader API refactor.

Files changed (8) +499 / -40

Enhancement (1) +37 / -0
render_sync.pyRender telemetry degradation independently from policy status +37/-0

Render telemetry degradation independently from policy status

• Extracts synchronization summary rendering and adds a delayed-telemetry row. Degraded telemetry changes the panel border to yellow without obscuring policy application results.

src/codex_plugin_scanner/guard/cli/render_sync.py

Bug fix (2) +131 / -15
optional_telemetry_sync.pyIsolate optional telemetry failures from policy progress +121/-0

Isolate optional telemetry failures from policy progress

• Introduces a coordinator that normalizes pain-signal and guard-event outcomes into stable status and reason fields. It preserves partial progress, redacts raw failure details, and rethrows typed or HTTP 401/403 authorization failures.

src/codex_plugin_scanner/guard/runtime/optional_telemetry_sync.py

runner.pyRoute optional uploads through the telemetry coordinator +10/-15

Route optional uploads through the telemetry coordinator

• Runs pain-signal and guard-event uploads through the new isolation layer and merges their results into the sync summary. Pain-signal transport errors now carry completed-page counts so successful cursor progress survives later failures.

src/codex_plugin_scanner/guard/runtime/runner.py

Refactor (1) +2 / -24
render.pyDelegate sync summary rendering to a focused module +2/-24

Delegate sync summary rendering to a focused module

• Replaces the inline synchronization panel implementation with the extracted 'render_sync_summary' function while retaining subsequent ecosystem rendering.

src/codex_plugin_scanner/guard/cli/render.py

Tests (3) +302 / -1
decision-diff-report.jsonRegenerate decision-report source fingerprints +2/-1

Regenerate decision-report source fingerprints

• Refreshes command-corpus source hashes after the synchronization changes while retaining the established decision-report baseline.

tests/fixtures/guard-command-corpus/decision-diff-report.json

test_policy_telemetry_isolation.pyVerify policy progress survives telemetry failures +156/-0

Verify policy progress survives telemetry failures

• Covers retained policies and acknowledgements, successful telemetry retries, secret-free reason reporting, CLI separation, and stale event-count prevention. It also verifies typed and HTTP 401/403 authorization failures remain fatal even when messages contain '429'.

tests/test_policy_telemetry_isolation.py

test_policy_telemetry_transport.pyVerify telemetry cursor and pagination retry behavior +144/-0

Verify telemetry cursor and pagination retry behavior

• Exercises real pain-signal and guard-event transport paths for endpoint, rate-limit, service, and connection failures. Tests confirm completed pages remain counted and only pending records are retried.

tests/test_policy_telemetry_transport.py

Documentation (1) +27 / -0
policy-sync-telemetry-outages.mdDocument degraded telemetry synchronization behavior +27/-0

Document degraded telemetry synchronization behavior

• Explains independent synchronization outcomes, stable telemetry reason codes, cursor-based retries, and CLI presentation. Clarifies that authorization, endpoint trust, and plan failures still abort synchronization.

docs/guard/policy-sync-telemetry-outages.md

Comment thread src/codex_plugin_scanner/guard/cli/render.py
Comment thread src/codex_plugin_scanner/guard/runtime/runner.py
Comment thread src/codex_plugin_scanner/guard/runtime/optional_telemetry_sync.py
Comment thread src/codex_plugin_scanner/guard/runtime/runner.py
@greptile-apps

greptile-apps Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR should not merge until a successful pain-signal upload followed by cursor-persistence failure can no longer cause duplicate telemetry on the next sync.

Fix All in CodexFindings

  1. P1 Cursor failures duplicate telemetry ▶

Summary

This PR separates optional pain-signal and Guard-event upload failures from receipt and signed-policy progress, adds stable telemetry status fields, and renders delayed telemetry independently in CLI sync output.

  • Retains accepted policy state and acknowledgements when optional telemetry transport fails.
  • Preserves completed pain-signal page counts for explicit upload failures and retries pending pages.
  • Keeps authorization, endpoint-trust, and plan failures on the required failure path.
  • Adds transport, policy-isolation, retry, redaction, and CLI rendering coverage.
  • Documents the additive sync result fields and outage behavior.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Receipt and policy sync] --> B[Validate and activate signed policy]
  B --> C[Persist policy acknowledgement]
  C --> D[Pain-signal upload]
  D --> E[Guard-event upload]
  D -->|Upload outage| F[Record telemetry degradation]
  E -->|Upload outage| F
  D -->|Authorization or trust failure| G[Fail sync]
  E -->|Authorization or trust failure| G
  F --> H[Return successful core progress]
  H --> I[Retry pending telemetry on next sync]
Loading

Reviews (1) · Last reviewed commit: "fix: preserve policy progress during opt..."

Comment thread src/codex_plugin_scanner/guard/runtime/optional_telemetry_sync.py

Copy link
Copy Markdown
Member Author

Review record retained during sensitive-history cleanup. The earlier automated review marked invalid-event response handling, delayed-upload visibility, and permanent telemetry rejection handling resolved.

The permanent-rejection requirement distinguishes retryable availability failures from HTTP400/413 rejection so ordinary sync cannot repeatedly submit an unchanged invalid batch. Existing renderer and runner size findings were reviewed against their parent: both modules shrink, and new regression coverage lives in separate files. This replacement preserves the review record; final-head CI remains a separate merge requirement.

@kantorcodes kantorcodes changed the title fix(guard): preserve policy progress during telemetry outages fix(sync): preserve policy progress during optional upload failures Sep 17, 2026
@gitar-bot

gitar-bot Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Code Review ✅ No issues found

🔴 High risk · Broad runtime changes alter policy synchronization, telemetry failure handling, and Cloud Review consent status

Preserves policy progress and authorization state when optional uploads are delayed or fail, keeping completed upload counts and clear status for each affected upload. No issues found.

Review coverage

📋 Rules No rules evaluated

🧪 Functional validation Not enabled · Set up

Options ✅ Auto-apply

✅ Auto-apply is on → Gitar will commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

✅ Auto-apply Compact
gitar auto-apply:off         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

@zerocodefast zerocodefast added the gitar-managed Enables Gitar automatic repair for a pull request label Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gitar-managed Enables Gitar automatic repair for a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants