Record policy-run failures as durable, actionable events (Review Flow PR 1) - #7269
Merged
EthanHealy01 merged 20 commits intoAug 10, 2026
Merged
Conversation
…ents Adds a team-scoped record of why a policy run failed, surfaced in the portal with the triage actions each failure kind allows. A failure kind registry (FailureKind) describes what can go wrong as data: a stable id, i18n keys, an English fallback, and four facets (stage, severity, remedy, scope). A classifier maps a thrown failure onto a kind by reading the errorCode out of the Problem Details body a tool returns, so classification keys off structured codes rather than exception message matching. Anything unrecognised becomes UNKNOWN, which means every failed run gets a durable record from day one. Actions are declared by a kind but implemented in FailureAction beans resolved by id, the same idiom already used for InputSource and PolicyOutputSink. A kind cannot be sent an action it does not declare, so an incoherent pairing is unreachable rather than merely unrendered. The record holds no document name or content: fileId is an opaque reference, and detail is stripped of anything path- or filename-shaped on the way in. PolicyExecutor's type-mismatch message now reports the extension instead of the filename, since that message becomes the stored detail. Reads and triage are leader-only, gated the same way policy editing is, and every read and write is scoped to the caller's own team from the authenticated principal.
EthanHealy01
requested review from
ConnorYoh,
Frooodle,
Ludy87,
jbrunton96 and
reecebrowne
as code owners
August 3, 2026 15:58
dosubot
Bot
added
size:XXL
stirlingbot
Bot
added
Java
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The HTTP integration test built its own SecurityFilterChain with CSRF disabled, which CodeQL flagged (java/spring-disabled-csrf-protection). The chain only existed so the auto-configured one would not answer 401 before the handler ran, so excluding the security auto-configurations removes both the 401 and the need to disable anything.
…y-safe Addresses review feedback on the failure-reporting PR. Folding a repeat into an existing incident was a read-modify-save of a detached entity, so a dismiss landing between the read and the save was reverted to NEW and concurrent folds lost occurrence counts. Both steps are now guarded UPDATE statements against the row's current values. A row that vanished between the dedup read and the fold is re-inserted rather than surfacing as an error the recorder swallows. Status transitions are guarded in the database too, so two racing closes resolve to one winner, and the pre-read that only classified the refusal is gone. AcknowledgeAction no longer hand-rolls that logic. Other fixes: - kindId filters in the query, before the limit, so a filtered page is no longer empty while matching rows exist - saveAndFlush on insert, so the duplicate-key violation lands in the catch rather than at a later commit - truncate() caps at 2000 including the ellipsis and never splits a surrogate pair - byErrorCode indexes once instead of scanning; a code claimed twice fails the boot instead of resolving by declaration order - facet enums promoted to top level, matching the other persisted enums - status query parameter bound by Spring's converter - redaction javadoc states plainly that it is best-effort Tests: FileRunEventStoreDbTest runs the real JPQL on a real database, since every existing test used the in-memory fake and the queries were unverified. Four tests that could not fail are fixed or deleted, and the fake now honours Pageable, which is what made the limit tests meaningless.
"I'll unlock this" promised something no action performs. Acknowledge only moves the row's status; nothing collects a password and nothing retries, because the failed document is never kept server-side. The kind now offers the generic Acknowledge alongside "Skip this file". Unlocking can be offered once there is a document to unlock.
EthanHealy01
changed the title
Contributor
|
This purports to be dev only but looks like it will show up on live (Claude find to investigate) |
Contributor
|
The PR desc claims Repeat folding but that doesn't work |
Contributor
|
Needs autorefreshing via polling, up to you if in this PR or a followup |
Contributor
|
The filename redaction breaks on files with brackets, spaces and probably in a bunch of other scenarios |
…builds Three review findings. Folding did not work. Both kinds ended up keyed on the run id — a FILE-scoped kind fell back to the run because nothing populated file_id, and UNKNOWN is run-scoped — and every sweep starts a new run. So a file that failed on every sweep opened a new incident every sweep, while several files failing in one run collapsed into a single row. Input sources now pass their own reference for the document, hashed via IdentityHasher (a folder identity is a path, and a path is a filename), and it is carried ResolvedInput -> PolicyRun -> recorder -> file_id. Incidents key on the document, so a repeat folds and distinct files stay distinct. This also fills in the column that was always NULL. The failures view is no longer mounted outside dev. Only its debug panel was gated before, so the section itself rendered in a build; the endpoints stay live and leader-gated, but the surface is unfinished. Vite folds the guard, so neither the view nor its fetch ships. The list polls every 30s, paused while the tab is hidden and stopped once the route answers 404 or 403. Failures arrive from background sweeps, so without it the list silently went stale.
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
The guard treated any error as permanent, so one dropped connection or a restarting server switched the list's refresh off for the rest of the session, with nothing to turn it back on. Only 403 and 404 stop it now: a build without the failure registry has no such route, and a member who is not a team leader may not read the queue. Everything else is transient and polling rides it out. Also formats the file, which is what failed frontend-validation.
stirlingbot
Bot
added
the
has conflicts
The values mixed two questions. EDITOR and API answer "where was this started", PROCESSOR answers "what ran it", and they overlap: a policy kicked off from the editor is both, and was recorded as PROCESSOR, which reads as unattended next to a row that names the person who hit it. Origin is now the run type alone: TOOL for a single tool call with no policy around it, POLICY for anything the policy engine ran however it was triggered. Where it came from is already answered by the fields beside it — actor names the person for an attended run, sourceId names the folder, bucket or webhook for an unattended one. API is dropped, having been an answer to the other question. PIPELINE is declared without a producer: the watched-folder pipeline that predates policies records no failures at all, and instrumenting it is its own piece of work.
…-transit-and-notifications # Conflicts: # frontend/editor/src/portal/queries/keys.ts
stirlingbot
Bot
removed
the
has conflicts
Raised in review, twice. The pattern only handled a single hyphenated ASCII token with one extension, which is not what documents are called. Everything below leaked in full or in part: report (final).pdf, severance-agreement.pdf.gz, termination.tar.gz, 履歴書.pdf, отчёт-зарплата.pdf, payslip%20march.pdf. It now matches by shape over unicode letters and digits, allows the punctuation names carry (brackets, %, &, apostrophes) and takes up to three stacked extensions. An all-digit extension still is not one, so v2.14.2 survives, and the lookarounds still keep it off dotted identifiers so a stack trace does. Spaces are crossed only when the name is delimited by a quote, bracket or path separator. An undelimited spaced name is indistinguishable from the sentence around it, so "Failed on Q3 Layoff List.pdf" loses the name but keeps its leading words rather than collapsing the whole message to "<file>". That limit is now stated in the javadoc instead of the old wording, and pinned by a test.
Agreed with Anthony that storing a name is not a concern today and proper detection comes later, so the shape-matching stays and the gap is written down rather than engineered around. Adds the TODO on both the pattern and the test that pins the gap, naming the durable fix: stop forwarding a downstream tool's message verbatim, keep our own wording plus the error code, and only pass through a body whose kind we classify and whose text we therefore know carries no name. Widens the cases the test covers to quoted names, Windows paths, ampersands and an uppercase extension, and makes the partial case assert what actually survives instead of only what does not.
Contributor
Author
|
Dev-only issue : now gated at the mount, so the view and its fetch are dropped from a build. Folding issue : input sources pass a hashed document identity through to file_id, so repeats fold on the document and distinct files stay distinct. Polling concern : 30s, paused when the tab is hidden, stops on 403/404. Redaction issue : now handles brackets, spaces, Unicode, %, & and stacked extensions, with a test per case. One known gap: an undelimited spaced name keeps its leading words, since crossing spaces without a delimiter swallows the sentence too. Agreed with Anthony to accept that for now; TODO on the pattern and the test. |
jbrunton96
reviewed
Aug 6, 2026
26 lines of javadoc for one field was too much. The detail of what is and is not caught belongs in RecordFailurePrivacyTest, which asserts it rather than claims it, so the comment now points there. Keeps the TODO, reframed around James's point: we own the producer, so the durable fix is to store the parsed Problem Details fields instead of the stringified exception, rather than reading our own structured data back out of prose.
It was private to RecordFailure, which is the wrong home for a rule that belongs wherever a document name might be written down. policy_processed_files already stores the full path in plaintext, and that is the next caller. Moves the pattern to common as FilenameRedaction.attemptRedaction, named for what it is rather than what it guarantees, with the cases moved alongside it. RecordFailurePrivacyTest keeps one check that a stored message goes through it at all, which is the part that belongs to the failure package. Also stops storing the downstream description for a kind we recognise: we already have its own copy, so the raw text only adds somewhere for a name to hide. Unrecognised failures keep theirs, since it is the only thing telling a reviewer what went wrong.
Per review: these are the user's own errors about their own files, and hiding parts of a message makes a row harder to act on without making it meaningfully safer. A name is also already kept elsewhere, so redacting only here bought consistency with nothing. Drops the redaction pattern, the shared utility and their tests, and stops suppressing the description for kinds we recognise. What remains of the privacy contract is the part that never depended on pattern-matching: no name column, and a dedup key built only from opaque identifiers.
reecebrowne
previously approved these changes
Aug 7, 2026
EthanHealy01
enabled auto-merge
August 7, 2026 19:20
Mounting the failures section on the Documents view made the a11y gate fail on four rules: aria-prohibited-attr, color-contrast, empty-table-header and nested-interactive. None of them are ours. Every failing node is in the review queue and the filter pills that were already there -- an aria-label on a plain span, a header cell with no text, table rows carrying role="button" around real buttons, and the active pill's accent on its own tint. What changed is when axe runs: the section adds another query, the table finishes rendering before the scan instead of after it, and violations that were always in the markup stop hiding behind a race. Scanning the same story with the section inert reports nothing, which is what the baseline was recorded against. So the gate is flaky rather than newly broken, and it flaps both ways: the same commit scans clean on one run and reports all four on the next. Both themes get the full set, so a run that happens to catch the table cannot fail a branch that never touched it. Baselining rather than fixing: two are one-liners, but the row role and the accent-on-tint contrast are a table restructure and a theme change, neither of which belongs in a PR about recording failures. Filed separately. The gate reports a baselined story that comes up clean, so it will ask for this to be ratcheted back once those land.
…g them Follow-up to the previous commit, which recorded four violations rather than fixing any. Three had real causes and real fixes: aria-prohibited-attr — the sensitive-document padlock is a bare <span> with an aria-label, and aria-label does nothing on a span with no role, so the padlock was silent. Given role="img" so the label is allowed and the icon reads as one thing. empty-table-header — the trailing column of row controls used header: "", leaving every cell under it unlabelled. Table columns can now mark a header as visually hidden, so the column keeps a name without putting a heading above it. Applied to the same blank header in the pipelines and sources tables, which had it too. nested-interactive — Table puts role="button" and tabIndex on a row when onRowClick is set, and the documents and policy rows also contain their own buttons. A button may not contain other controls, and a <tr role="button"> stops being a row at all, so the table structure was lost as well. Those two tables now say so, and their rows keep the click as a mouse shortcut while the button inside carries the keyboard path to the same action. The rows in the pipelines and sources tables hold nothing but a chevron, so they stay buttons and keep their keyboard access. That leaves color-contrast, which stays baselined. The active filter pill paints --c-primary on --c-primary-tint: 3.06:1 against the 4.5:1 it needs. The existing text-weight accent (--c-primary-hover, 80% toward black) only reaches 4.47:1, so it needs a token of its own, tuned per theme because the tint is dark on a dark base and the text has to go the other way. That is a theme change affecting every pill tab in the app, not something to slip into this branch.
reecebrowne
approved these changes
Aug 10, 2026
ConnorYoh
approved these changes
Aug 10, 2026
EthanHealy01
deleted the
feature/file-run-info-transit-and-notifications
branch
August 10, 2026 17:27
EthanHealy01
added a commit
that referenced
this pull request
Aug 13, 2026
Failures have been durable and actionable since #7269 and reportable from the editor since #7296, but the only way to see one was a dev-only list at the bottom of the processor's Documents view. This puts them in front of the person who can act. An action is declared as data, not rendered by rules the client hard-codes. A FailureKind offers each action with an audience (the file's owner, the team's reviewer, or anyone who can see the row) and a slot, and the server resolves both against who is reading. Ownership is derived per reader from the row's actor rather than stored. So a new failure kind ships as a registry entry plus copy, and needs no frontend change at all. Which actions the server runs and which the client runs are now distinct: FailureActionId carries an Execution facet, the registry only requires a bean for a server action, and dispatching a client action is refused. Retry, decrypt-and-retry, view file and view in processor are the client's; dismiss stays the server's. The client hides an action it cannot perform rather than showing it disabled, and surfaces the server's reason as the row's note instead. A greyed-out button that can never work reads as false hope, and a note says the same thing honestly. Notifications derive from failures on read rather than living in a table of their own: one source today, and a table would need a write path, retention and a per-user read model before it earned itself. Every id on the notification endpoints is prefixed, so the bell can never hand a raw failure id to a failure endpoint. An attended policy run now records which document it was about, which is what lets a repeat fold onto one incident instead of opening a new one per upload, lets deleting the file clear its failure, and lets the owner open the document from the row. The failures list in the processor stays gated behind import.meta.env.DEV, and the bell's "view in processor" action is gated the same way, so it cannot point at a section that is not mounted. Both lift together when failures get their own review screen. The read scope itself is unchanged here: #7477 decides who may see which row, and this commit only decides what each reader is offered about a row they can already see.
EthanHealy01
added a commit
that referenced
this pull request
Aug 13, 2026
Failures have been durable and actionable since #7269 and reportable from the editor since #7296, but the only way to see one was a dev-only list at the bottom of the processor's Documents view. This puts them in front of the person who can act. An action is declared as data, not rendered by rules the client hard-codes. A FailureKind offers each action with an audience (the file's owner, the team's reviewer, or anyone who can see the row) and a slot, and the server resolves both against who is reading. Ownership is derived per reader from the row's actor rather than stored. So a new failure kind ships as a registry entry plus copy, and needs no frontend change at all. Which actions the server runs and which the client runs are now distinct: FailureActionId carries an Execution facet, the registry only requires a bean for a server action, and dispatching a client action is refused. Retry, decrypt-and-retry, view file and view in processor are the client's; dismiss stays the server's. The client hides an action it cannot perform rather than showing it disabled, and surfaces the server's reason as the row's note instead. A greyed-out button that can never work reads as false hope, and a note says the same thing honestly. Notifications derive from failures on read rather than living in a table of their own: one source today, and a table would need a write path, retention and a per-user read model before it earned itself. Every id on the notification endpoints is prefixed, so the bell can never hand a raw failure id to a failure endpoint. An attended policy run now records which document it was about, which is what lets a repeat fold onto one incident instead of opening a new one per upload, lets deleting the file clear its failure, and lets the owner open the document from the row. The failures list in the processor stays gated behind import.meta.env.DEV, and the bell's "view in processor" action is gated the same way, so it cannot point at a section that is not mounted. Both lift together when failures get their own review screen. The read scope itself is unchanged here: #7477 decides who may see which row, and this commit only decides what each reader is offered about a row they can already see.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
PR 1 of the failure-notification work: a durable, team-scoped record of why a policy run failed, surfaced in the portal with the triage actions each failure allows.
Today a failed policy run is not quite invisible, but it is unusable: the ledger marks the file
ERROR, and the audit aspect keeps the exception message and status code. Nothing classifies either one, nothing surfaces them, and neither offers a next step. If the file came from a folder, bucket or webhook there is also no user watching, so nobody learns it never made it through. This adds the record and the read surface; the remediation that acts on documents comes later (see below).What this does
A failure kind registry as data.
FailureKinddescribes what can go wrong: a stable wire id, i18n keys, an English fallback, and four facets the review surface needs (Stage,Severity,Remedy,Scope). It is shaped like the existingExceptionUtils.ErrorCodeand links to that vocabulary rather than replacing it.Classification off structured codes, not message matching. Policy steps dispatch over loopback HTTP, so a tool's 4xx arrives as a
RestClientResponseExceptionwhose body is the Problem Details document carryingerrorCode.FailureClassifierreads that. Anything unrecognised becomesUNKNOWN, which is the point: every failed run gets an addressable record from day one, and which kinds to promote next is answered by production frequency rather than guesswork.Actions declared by a kind, implemented as beans. A kind lists the
FailureActionIds it offers; behaviour lives inFailureActionbeans resolved by id — the idiom this codebase already uses forInputSource,PolicyOutputSinkandPolicyTrigger. A kind cannot be sent an action it never declared (400), so an incoherent pairing is unreachable rather than merely unrendered. A new kind ships as a registry entry plus copy: no new endpoint, no UI change.Repeat folding. Recording folds a genuine repeat into the existing incident instead of inserting again, keyed on
(team_id, dedup_key). That matters for a snapshot-mode source that re-lists every file on each sweep: the same broken file is one incident, not one per sweep. Distinct files keep distinct rows. The unique constraint is enforced by the database, and a writer that loses the insert race folds into the winner's row.One granularity caveat worth naming: nothing populates
file_idin this PR, so every row has it NULL. A FILE-scoped kind therefore dedups onpolicy + runrather thanpolicy + file. That still yields one row per document for the sources shipped here, because the folder, S3 and webhook sources each start one run per file; it stops holding as soon as a single run carries several documents, which is why editor-origin reporting (item 3 below) populatesfile_id.No document identity is stored. No file name, no content.
fileIdis an opaque reference only the owner's own client can resolve locally.detailkeeps the raw message (the only diagnostic anUNKNOWNfailure has) with anything path- or filename-shaped stripped on the way in, capped at 2,000 characters.PolicyExecutor's type-mismatch message now reports the extension rather than the filename, since that message becomes the storeddetail.Access. Reads and triage are leader-only, gated exactly the way
PolicyControllergates policy editing, with the single-user carve-out when login is disabled. Every read and write is scoped to the caller's own team from the authenticated principal — there is no team parameter on the API.Self-hosted needs no migration: the table is created from the entity by
ddl-auto=update, as with every other table.What this does not do yet
UNKNOWN, and why there is no Approve/Release yet.INPUT_PASSWORD_PROTECTEDandUNKNOWN. Everything else classifies asUNKNOWNand shows its raw message.PROCESSOR.FailureOrigin.EDITORandAPIexist in the enum but nothing writes them.import.meta.env.DEV, so it ships in no production bundle. The endpoints are live and gated.file_run_events. Tracked separately.PolicyInputRequiredExceptionand the engine'ssuspend()exist but nothing throws it, so a run cannot pause to ask for a password today.Stirling-PDF-SaaS(CREATE TABLE IF NOT EXISTS stirling_pdf.file_run_events), per the convention documented atapp/saas/src/main/resources/application-saas.properties:21.What follows in later PRs
UNKNOWNcatch-all with a named notification in the review UI.How to test
Needs a SaaS or proprietary build with login enabled, and an account that leads a team.
.csvto.pdf).Expect two rows:
E004, with the kind's own labels "I'll unlock this" and "Skip this file" rather than generic wording.UNKNOWN(E001is not claimed by a kind yet), showing its raw message with generic Acknowledge / Dismiss.Neither row contains a file name anywhere, including in the raw message. Press Show raw JSON to read exactly what the server returned. Acting on a row transitions it and comes back with both buttons disabled and a reason.
Re-running the same batch increments the occurrence count on the existing rows rather than adding new ones; two different password-protected files produce two separate rows.
Checklist
General
Documentation
Translations (if applicable)
scripts/counter_translation.pyUI Changes (if applicable)
Testing (if applicable)
task checkto verify linters, typechecks, and tests pass