Harden DSL execution reliability: lock down show path, unify abort handling, isolate thread-local state#3
Harden DSL execution reliability: lock down show path, unify abort handling, isolate thread-local state#3kmdvs wants to merge 47 commits into
Conversation
|
Follow-up commit pushed: Fix DSL regression semantics and stabilize test reliability. This addresses failing tests uncovered after the initial PR push. |
This extends the DSL hardening work with a full pipeline evaluation and diagnosis system. Includes: - PipelineEvaluator: deterministic evaluation from persisted statements (no DSL rerun) - PipelineInterpreter: stable metrics (facts layer) - PipelineDiagnosis: rule-based diagnosis with Wringer signal integration - SemanticInterpreter: step-level semantic support Trace improvements: - Enhanced DslTraceCollector and TraceFormatter - Step-level visibility (input/output/error) - Improved debugging for DSL scripts Wringer integration: - WringerClient introduced - Signals injected into diagnosis (no network coupling in diagnosis layer) API: - /events/:id/pipeline_health endpoint - Returns status, category, message, suggested_action, metrics, details UI: - Updated statements view to support trace inspection Tests: - Full TDD coverage for interpreter, diagnosis, evaluator, controller - Priority rules enforced (error > wringer > data_loss > navigation > extraction) Notes: - Steps remain internal (not exposed in API) - Wringer is contextual (not part of pipeline execution)
|
@codex make a deep review of the latest changes. You are a senior Ruby on Rails engineer reviewing a DSL pipeline observability system. Focus ONLY on correctness, architecture, and edge cases. Do NOT comment on style. This PR introduces:
Review the diff with focus on:
Do NOT summarize the code. |
![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: e572aa6d44
ℹ️ 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".
| else | ||
| { body: resp.to_s, http_code: 200, final_url: nil } |
There was a problem hiding this comment.
safe_wringer_call
The new safe_wringer_call stringifies any non-HTTP return value via resp.to_s and returns that string. wringer_received_404? passes a boolean-returning block through this helper, so the normal false path becomes "false", which is truthy and makes !!result return true. In RefreshWebpageJob#perform, that causes successful non-404 lookups to be treated as stored 404s and the webpage gets destroyed after refresh.
Useful? React with 👍 / 👎.
| body = @safe_wringer_call.call do | ||
| @agent.get_file(@use_wringer.call(url, render_js, scrape_options)) |
There was a problem hiding this comment.
get_file
WringerClient still calls @agent.get_file(...), which only yields the response body. In safe_wringer_call, those results go through the non-response branch and are normalized as http_code: 200, final_url: nil, so every rule in config/wringer.yml that depends on the real status or redirect target (redirect_to_listing, http_404, http_5xx) is unreachable for DSL fetches. A listing redirect or real 404 therefore looks like a normal HTML page and never propagates Wringer status.
Useful? React with 👍 / 👎.
| if data.is_a?(Array) && data.first == "abort_update" | ||
| info = data.second || {} | ||
| stat.errors.add(:base, "Scrape aborted (#{info[:error_type]}): #{info[:error]}") | ||
| return | ||
| abort_error_message = "Scrape aborted (#{info[:error_type]}): #{info[:error]}" | ||
| stat.errors.add(:base, abort_error_message) | ||
| error_messages << abort_error_message |
There was a problem hiding this comment.
abort_update
This branch no longer returns after an abort_update. For xsd:anyURI statements, the method now falls through into format_datatype, which iterates the abort payload array and tries to reconcile 'abort_update' (and even the error hash) as if they were scraped URLs. On a Wringer failure for ticket/location links, that produces bogus KG lookups and can overwrite the cache with corrupted URI data instead of preserving the fetch failure.
Useful? React with 👍 / 👎.
| def evaluate | ||
| steps = pipeline_steps | ||
| metrics = Dsl::PipelineInterpreter.new(steps).metrics | ||
| diagnosis = Dsl::PipelineDiagnosis.new(metrics: metrics, wringer: DEFAULT_WRINGER).result |
There was a problem hiding this comment.
PipelineEvaluator always instantiates PipelineDiagnosis with DEFAULT_WRINGER, even though the runner and trace now capture Wringer status as { error_type, retry, cache }. That means /events/:id/pipeline_health can never emit :wringer_failure for Cloudflare/queue/404 cases; those signals are dropped before diagnosis and the endpoint falls back to a generic :error or even :healthy result based only on statement cache.
Useful? React with 👍 / 👎.
| type: infer_step_type(statement.source&.algorithm_value), | ||
| output: normalize_output(statement.cache), |
There was a problem hiding this comment.
Each Statement is collapsed into one synthetic step whose type is just the first prefix of the entire DSL string. For any real algorithm like xpath=...;url=...;xpath=..., the interpreter only ever sees xpath, so has_navigation, suspicious_navigation, and failure_step cannot represent later navigation or extraction failures. The new pipeline health endpoint is therefore systematically misleading for multi-step statements, which are the common pipelines this feature is meant to explain.
Useful? React with 👍 / 👎.
…gration - unify DSL execution, tracing, and pipeline evaluation layers - add pipeline diagnosis and interpreter improvements - enhance wringer client with richer metadata (redirects, signals) - stabilize DSL runner semantics and error handling - improve trace formatter with severity-aware output - align tests with DSL contract and reliability guarantees Establishes a consistent execution model across DSL runner, trace collection, pipeline evaluation, and wringer integration. Foundation for improved debugging and observability.
…ive/trace) - remove Fast mode (incomplete and confusing) - rename Mode → View and add explicit Trace status - unify semantic styling across all views (state-based CSS) - make wringer metadata always visible (redirects, links, probe) - align Debug with Interactive (expanded) while reducing noise - remove raw trace from Interactive (separation of concerns) - improve condition labeling ([If found]) - add color-coded semantic states (ok/warning/error) - improve readability with consistent indentation and layout Result: - Debug = full pipeline overview (flat, readable) - Interactive = step-by-step debugger - Trace = technical execution log - Clean = DSL only Transforms trace viewer into a coherent debugging interface.
|
@codex You are a very senior Ruby on Rails engineer reviewing this branch after a substantial stacked change set. Please perform a deep code review of the current branch, with special attention to regressions, architectural coherence, and hidden edge cases. Context:
Review goals:
Output format:
Important:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9920ddc445
ℹ️ 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".
| new_url = Dsl::UrlResolver.extract(raw) | ||
| if new_url.blank? | ||
| Rails.logger.debug { "[DSL] skipped invalid URL from #{raw.inspect}" } | ||
| return skip |
There was a problem hiding this comment.
When Dsl::UrlResolver.extract returns nil, this path returns :skip, which makes handle_url_step silently keep prior state (arr, @url, and @page) instead of failing. In pipelines where url=$array.first becomes invalid after filtering, later extraction steps run against stale page context and can write incorrect data while appearing successful. Return an abort_update payload (as the previous implementation did for invalid URLs) so refresh surfaces a real error instead of silently degrading.
Useful? React with 👍 / 👎.
| flash[:notice] = "Statement was successfully refreshed." | ||
| end | ||
|
|
||
| redirect_to @statement |
There was a problem hiding this comment.
The action now unconditionally redirects, so PATCH /statements/:id/refresh.json no longer returns structured JSON success/error payloads and instead emits an HTML redirect. Any API caller relying on the documented .json endpoint will lose machine-readable refresh results and error details. Reintroduce respond_to handling so HTML keeps redirect behavior while JSON responses are returned with appropriate status codes.
Useful? React with 👍 / 👎.
| def abort_structure?(obj) | ||
| obj.is_a?(Array) && | ||
| obj.length == 2 && | ||
| obj.first == "abort_update" && |
There was a problem hiding this comment.
safe_wringer_call now supports policy actions beyond abort_update (for example, custom actions like skip), but WringerClient only treats ['abort_update', payload] as a control result. Any other action tuple is misclassified as a successful fetch body, so policy decisions are ignored and downstream parsing can run on non-HTML control payloads. Normalize and branch on the action token generically (or explicitly handle all supported actions) instead of hard-coding only abort_update.
Useful? React with 👍 / 👎.
| get 'events/:id/pipeline_health', | ||
| to: "events#pipeline_health", | ||
| as: :event_pipeline_health |
There was a problem hiding this comment.
This route captures the event identifier in a standard :id segment, which does not safely support identifiers containing / (common for URI-shaped RDF IDs). Requests for such events will fail routing before reaching EventsController#pipeline_health, making the endpoint unusable for a subset of real identifiers. Use a query parameter or a glob segment (e.g., *id) so full URI-like IDs are routable.
Useful? React with 👍 / 👎.
- Extract inline CSS and JS into dedicated asset files - Add robust JS initialization (DOMContentLoaded + turbo:load) - Scope JS execution to statements/show page - Simplify view by relying on TracePresenter (visibility, trace state) - Consolidate top-level locals and improve readability No functional changes; improves separation of concerns and maintainability
- Extract inline CSS and JS into dedicated asset files - Add robust JS initialization (DOMContentLoaded + turbo:load) - Scope JS execution to statements/show page - Simplify view by relying on TracePresenter (visibility, trace state) - Consolidate top-level locals and improve readability No functional changes; improves separation of concerns and maintainability
|
@codex You are a very senior Ruby on Rails engineer performing a deep code review. Context: Wringer (fetch) → DSL runner → pipeline diagnosis → trace → presenter → view Recent changes include:
Your task: REVIEW AREAS
OUTPUT FORMATReturn findings grouped by severity: HIGH:
MEDIUM:
LOW:
IMPORTANT RULES
GOALThe goal is to ensure:
This is not a lint — this is a production-grade system review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5300a07a6f
ℹ️ 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".
| step_payload[:w] = { | ||
| i: event[:wringer][:inherited], | ||
| et: event[:wringer][:error_type], | ||
| r: event[:wringer][:retry], | ||
| c: event[:wringer][:cache], | ||
| u: event[:wringer][:unreachable], | ||
| r404: event[:wringer][:received_404], | ||
| se: event[:wringer][:system_error], | ||
| pa: event[:wringer][:policy_action], | ||
| s: event[:wringer][:signals], | ||
| h: event[:wringer][:hints] | ||
| }.compact |
There was a problem hiding this comment.
The v2 session formatter drops final_url/redirect_chain when compacting wringer data, so redirect metadata emitted by the fetch layer is lost before it reaches StatementsHelper#interactive_redirect_info and the trace UI. In practice, a run can correctly detect/record a redirect in the runner, but after for_session_v2 + expand, the view cannot tell that redirection happened, which breaks the new redirect propagation contract and makes debugging navigation failures much harder.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,52 @@ | |||
| class TracePresenter | |||
| DEFAULT_MODE = 3 | |||
| VALID_MODES = [1, 2, 3, 4].freeze | |||
There was a problem hiding this comment.
TracePresenter rejects mode 5, but the options flow explicitly sets trace_view_mode=5 for Debug and the statement view has a dedicated debug branch (is_debug). Because this whitelist excludes 5, selecting Debug always falls back to mode 3, so the debug rendering path is effectively unreachable for users.
Useful? React with 👍 / 👎.
| end | ||
|
|
||
| def effective_wringer | ||
| DEFAULT_WRINGER.merge(@wringer.slice(:unreachable, :received_404, :system_error, :policy_action)) |
There was a problem hiding this comment.
PipelineEvaluator strips wringer input down to four flags before calling PipelineDiagnosis, dropping fields like error_type, signals, and hints that diagnosis logic relies on for fetch-specific classification/messages. When callers pass richer wringer status (for example network_status: "failed" or WringerFetchError), this truncation can misclassify failures as non-fetch issues or healthy states.
Useful? React with 👍 / 👎.
- Preserve redirect metadata in trace formatter (fu, rc) - Enable debug mode (accept trace_view_mode=5) - Forward full wringer context to pipeline diagnosis
…ition - introduce TracePresenter as single interpretation layer - extract TraceFormatter for normalization - unify semantic(step) and step_output usage - decompose trace UI into partials (header, diagnosis, step) - remove legacy trace_demo view No behavior changes; improves consistency, safety, and maintainability
- improve DSL runner reliability - refine pipeline diagnosis logic - align tests with updated behavior No functional regressions
- add WringerClient abstraction - introduce shared source identity logic - add tests for wringer integration
- replace legacy flat DSL implementation with modular architecture: - Dsl::Core (algorithm execution) - Dsl::Tracing (trace collection and formatting) - Dsl::Parsing (content parsing) - Dsl::Support (url resolution, wringer client) - remove legacy DSL classes (DslAlgorithmRunner, DslRunner, etc.) - preserve DSL execution semantics and abort_update contract - update tests to target new module structure All existing DSL tests pass with no behavior change.
saumier
left a comment
saumier
left a comment
There was a problem hiding this comment.
Hi Kim. I am not able to review this. 385 file changes! Much too big and I don't understand enough to add value here.
|
@codex You are reviewing a Rails codebase on branch Context: Recent changes already added:
Important staging observations:
Files to inspect first:
Review goals:
Specific review questions:
Expected improvements to recommend if missing:
Output requested:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 786b0e7bc8
ℹ️ 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".
| wringer: Distillator::FetchService.build_wringer_status(control, raw_response), | ||
| http_code: nil, | ||
| raw_body: nil | ||
| } |
There was a problem hiding this comment.
Keep the original response body in raw_body for abort-control responses; right now control_result sets raw_body: nil, so downstream enrichment/store logic treats policy rejections as bodyless fetches, sets empty_body signals, and can misclassify a 200 HTML fetch rejected by content policy as an origin-empty failure. This directly affects cache diagnostics and transition blocking explanations for operator decisions. Populate raw_body from raw_response.body (when available) and add regression tests covering “HTTP 200 + non-empty HTML + abort_update” to ensure empty_body is not emitted.
Useful? React with 👍 / 👎.
| body_contains: | ||
| - "Error" | ||
| - "Erreur" |
There was a problem hiding this comment.
Tighten this rule because matching any page containing "Error" or "Erreur" is overly broad and will flag many valid pages, which then drives abort_update/cache: false policy and blocks refreshes despite successful transport. In production this can create widespread false failures and misleading operator blockers. Add stricter predicates (e.g., scoped phrases/selectors and/or HTTP/status constraints) plus tests showing a normal WordPress HTML page with incidental “error” text does not trigger this rule.
Useful? React with 👍 / 👎.
| <%= link_to "all websites", websites_path %> | | ||
| <%= link_to "webpages", webpages_path %> | | ||
| <%= link_to "sources", sources_path %> | |
There was a problem hiding this comment.
Reintroduce the contextual website breadcrumb immediately after “all websites” when website context exists; this change removed the website link entirely, so operators on transition/cache/detail pages lose the direct route back to the active website and can more easily navigate in the wrong context during rollout decisions. Render a context-aware link (and omit it when no context is available), then add integration coverage for transition detail and cache detail navigation.
Useful? React with 👍 / 👎.
| if row.website.distillator_mode == "shadow" | ||
| links << { | ||
| kind: :button, | ||
| label: "Run transition check", | ||
| url: distillator_transition_checks_path, |
There was a problem hiding this comment.
Avoid emitting mutating buttons from this shared action-link helper because it is used inside the Transition Report “Diagnostics” section; as written, shadow_report_actions injects a Run transition check button_to, which makes a diagnostic/read-only area perform writes and contradicts operator-safety expectations. Keep mutating actions only in the dedicated “Next action” area (or add a links-only mode for diagnostics), and add a view test asserting no form/button is rendered in Diagnostics.
Useful? React with 👍 / 👎.
| { | ||
| title: "Identity card", | ||
| rows: [ | ||
| { label: "Normalized URL", value: @cache_payload[:normalized_url], value_class: "cache-normalized-url" }, |
There was a problem hiding this comment.
Render the normalized URL as a clickable external link for HTTP/HTTPS values instead of plain text so operators can immediately verify source content when diagnosing cache failures; the current text-only field slows triage and obscures origin-vs-cache comparisons. Add scheme validation and render with target="_blank" and rel="noopener", plus tests for HTTP/HTTPS link rendering and non-HTTP values remaining plain text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc701d8180
ℹ️ 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".
| trace = session[:dsl_trace] | ||
| trace = trace.to_h if trace.respond_to?(:to_h) | ||
| trace = nil if trace == {} | ||
| @trace = safe_trace_copy(trace) |
There was a problem hiding this comment.
The show action always reads session[:dsl_trace] and renders it without checking which statement produced it. Since refresh writes a single shared session trace, a user can refresh statement A and then open statement B and see A’s trace attached to B, which can lead to incorrect debugging and operator decisions. Persist the statement id with the trace and ignore/clear traces that do not match @statement.id.
Useful? React with 👍 / 👎.
| message = "Webpage not found for URL: #{params[:url]}" | ||
|
|
||
| respond_to do |format| | ||
| format.html { redirect_to webpage_statements_path(url: params[:url]), alert: message } |
There was a problem hiding this comment.
When a refresh is requested for a URL that has no Webpage, this HTML branch redirects to webpage_statements_path with the same missing URL. That target action still dereferences webpage.statements without a nil guard, so the redirect turns the not-found case into a 500 instead of a graceful error response. Redirect to a safe page (or render a 404 page directly) for this branch.
Useful? React with 👍 / 👎.
| result = @dsl_binding.eval(sub(code, arr)) | ||
|
|
||
| # sync back DSL state | ||
| @url = Thread.current[:dsl_url] | ||
| @json = Thread.current[:dsl_json] | ||
|
|
||
| result |
There was a problem hiding this comment.
The ruby step now returns only eval’s return value, but many in-place Ruby operations can return nil when no change occurs (for example uniq!). In that case the pipeline state becomes nil even though Thread.current[:dsl_array] still contains the intended array, and downstream steps run against the wrong state. Return the updated thread-local array as a fallback so mutating ruby steps preserve pipeline data.
Useful? React with 👍 / 👎.
| build_session_v2( | ||
| normalized, | ||
| code_limit: 20, | ||
| error_code_limit: 80, | ||
| output_limit: 20, | ||
| initial_limit: 20, | ||
| include_duration: false | ||
| ) |
There was a problem hiding this comment.
for_session_v2 checks MAX_SESSION_BYTES for the first two compaction attempts but returns the third payload unconditionally. Large traces (especially with many steps and wringer metadata) can still exceed the configured session budget and then be written to session[:dsl_trace], risking cookie/session overflow failures during refresh. Re-check size after the final fallback and drop or aggressively trim if still too large.
Useful? React with 👍 / 👎.
| s: event[:wringer][:signals], | ||
| h: event[:wringer][:hints], |
There was a problem hiding this comment.
The v2 session formatter copies event[:wringer][:signals] and event[:wringer][:hints] directly into each compacted step without any size limits. Those diagnostics can be large nested payloads, so traces with only a few steps can still blow up session size and trigger cookie overflow despite code/output truncation. Summarize or cap these fields before assigning them into step_payload[:w].
Useful? React with 👍 / 👎.
|
|
||
| card.dataset.operatorContextInitialized = "true"; | ||
|
|
||
| const storedCardOpen = window.localStorage.getItem(CARD_KEY); |
There was a problem hiding this comment.
This direct localStorage read is outside any safety wrapper, so in environments where storage access is blocked (for example strict privacy settings or embedded contexts) it can raise SecurityError and abort the initializer. When that happens the operator context behavior breaks and can prevent later JS in the same load path from running. Use the same guarded pattern as readDomainState for all storage reads/writes.
Useful? React with 👍 / 👎.
3 participants
Summary
This PR hardens DSL execution reliability and safety in three focused commits, prioritized for production stability in a multi-threaded Rails/Sidekiq environment.
Changes
0257f34- Harden statement show path and extract webpage refresh orchestrationStatementsController#showno longer executes DSL during page load (removes unauthenticated request-time scraping/eval path).c4e085d- Unify abort-update helper semantics and fix non-trace DSL result handlingabort_updatecontract handling in helper paths.data, = run_dsl(...)truncated multi-value DSL outputs.b5b7677- Harden DSL runner control flow and isolate thread-local stateif_xpath/unless_xpath.SyntaxErrorhandling so malformed DSL returnsabort_updateinstead of raising.ensure-based thread-local restore/cleanup to prevent DSL state leakage across requests and Sidekiq jobs.Why this split
Each commit is atomic and reviewable by concern:
Test Coverage
Risk / Compatibility Notes
StatementsController#showno longer executes runtime DSL tracing.Follow-up (out of scope for this PR)
evalin transformation steps.Reviewer Checklist
GET /statements/:id.abort_updatehandling is consistent across helper/runner paths.