diff --git a/.gitignore b/.gitignore index d2b52ea6f8..4b34350f67 100644 --- a/.gitignore +++ b/.gitignore @@ -26,6 +26,7 @@ watchedFolders/ # also matches this frontend source component dir; keep the source tracked. !frontend/editor/src/proprietary/components/watchedFolders/ clientWebUI/ +policy-webhook-spool/ # Scratch dir used by local fixture-regeneration runs (see # app/proprietary/src/test/resources/db-migration-fixtures/README.md). # Holds downloaded JARs and disposable workdirs. Never committed. diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/EditorFailureReport.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/EditorFailureReport.java new file mode 100644 index 0000000000..b200b3d589 --- /dev/null +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/EditorFailureReport.java @@ -0,0 +1,47 @@ +package stirling.software.proprietary.failure; + +import java.util.List; + +/** + * A failure a user hit in the editor, reported by their own client. The editor calls tools directly + * rather than through the policy engine, so nothing server-side sees these unless the client says + * so. + * + *

Note what the client cannot supply: no team, no actor, no document name. The first two come + * from the authenticated session, and the third is never stored. + * + * @param operation the tool that failed, e.g. {@code remove-password} + * @param errorCode the code from the tool's Problem Details response, or null when there was none + * @param fileIds opaque client-side ids of the documents involved; empty when none is attributable + * @param detail the message the user saw + */ +public record EditorFailureReport( + String operation, String errorCode, List fileIds, String detail) { + + /** + * Cap on the files one report may name. Each one becomes a permanent incident and this endpoint + * is open to any authenticated user, so without a bound a single call can flood a leader's + * queue. 200 is several times the largest batch an editor session plausibly fails on, and the + * most the review queue shows in one page, so a real report never meets it. + * + *

An oversized report is refused whole rather than trimmed: see {@link + * FileRunEventController#report}. + */ + static final int MAX_FILE_IDS = 200; + + public EditorFailureReport { + fileIds = fileIds == null ? List.of() : List.copyOf(fileIds); + } + + boolean hasOperation() { + return operation != null && !operation.isBlank(); + } + + /** + * Counted before the blank ids are dropped, because this bounds the request rather than the + * rows it would produce. + */ + boolean namesTooManyFiles() { + return fileIds.size() > MAX_FILE_IDS; + } +} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java index d46f2d85e1..c9acd0d590 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FailureKind.java @@ -23,6 +23,10 @@ import lombok.Getter; *

Actions are declared here but implemented in {@link FailureAction} beans resolved by id, so a * new kind ships as a registry entry plus copy. Two members today: {@link #UNKNOWN} gives every * failed run a record, and kinds get promoted out of it as production shows what occurs. + * + *

A kind offers an acknowledgement only where there is something to acknowledge doing. + * With nothing to fix, "seen it" and "clear it" are the same decision, so the row offers only the + * one that clears it. */ @Getter public enum FailureKind { @@ -43,7 +47,6 @@ public enum FailureKind { FailureScope.RUN, noErrorCodes(), fallback("This run failed for a reason Stirling does not yet recognise."), - offer(ACKNOWLEDGE), offer(DISMISS)); private static final String KEY_PREFIX = "portal.failures.kind."; diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEvent.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEvent.java index cb6b3a452f..10f46cdbe2 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEvent.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEvent.java @@ -20,6 +20,7 @@ public record FileRunEvent( FailureOrigin origin, String policyId, String runId, + String sourceId, String fileId, String detail, String dedupKey, @@ -46,6 +47,7 @@ public record FileRunEvent( entity.getOrigin(), entity.getPolicyId(), entity.getRunId(), + entity.getSourceId(), entity.getFileId(), entity.getDetail(), entity.getDedupKey(), diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java index 49aab9aff6..a14f243678 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventController.java @@ -5,6 +5,7 @@ import java.util.List; import java.util.Map; import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; @@ -91,6 +92,50 @@ public class FileRunEventController { } } + @PostMapping("/reports") + @Operation( + summary = "Report a failure hit in the editor", + description = + "For failures the server never sees, because the editor calls tools directly." + + " Open to any authenticated user, unlike the read and triage endpoints:" + + " whoever's work failed can say so, and a leader reviews it. Rejected" + + " with 400 if it names more files than one report may carry.") + public ResponseEntity report(@RequestBody EditorFailureReport report) { + if (report == null || !report.hasOperation()) { + throw new ResponseStatusException( + HttpStatus.BAD_REQUEST, "operation is required to report a failure"); + } + // Refused whole rather than trimmed, and refused before the first write, so an oversized + // report leaves no rows at all. Trimming would hand a reviewer part of a set with nothing + // saying the rest existed, which is what the cap inside the service used to do. The limit + // is stated in the message because the editor reports in the background: a client author + // reading a log is the only person who will ever see this. + if (report.namesTooManyFiles()) { + throw new ResponseStatusException( + HttpStatus.BAD_REQUEST, + "a report may name at most " + + EditorFailureReport.MAX_FILE_IDS + + " files, and this one named " + + report.fileIds().size()); + } + service.report(report); + // No body: the editor reports and moves on, and has nothing to do with the row. + return ResponseEntity.noContent().build(); + } + + @PostMapping("/removed-files") + @Operation( + summary = "Close the incidents about files deleted from the editor", + description = + "Deleting the document leaves nothing to act on, so its incidents drop out of" + + " the queue while the rows stay for audit. Open to any authenticated" + + " user, and applies only to their own editor rows.") + public ResponseEntity filesRemoved(@RequestBody(required = false) RemovedFiles request) { + service.forgetFiles(request == null ? List.of() : request.safeFileIds()); + // No body: the editor is telling the server, not asking it anything. + return ResponseEntity.noContent().build(); + } + @GetMapping("/kinds") @Operation( summary = "List known failure kinds", @@ -136,6 +181,22 @@ public class FileRunEventController { /** Wrapped rather than a bare array so pagination can be added without breaking clients. */ public record FileRunEventsResponse(List events) {} + /** + * Files gone from the caller's editor. Opaque ids only, as everywhere else on this API. + * + *

Deliberately uncapped where a report is capped, because this creates nothing: it closes + * rows the caller already owns, so however long the list is, it can only ever touch incidents + * that already exist. Refusing an oversized one would also be the harmful direction here, since + * the editor says this once and never retries: those incidents would sit in the queue asking + * for attention about files that no longer exist. + */ + public record RemovedFiles(List fileIds) { + + List safeFileIds() { + return fileIds == null ? List.of() : fileIds; + } + } + /** Inputs an action declared it needs. Empty for both actions that exist today. */ public record ActionRequest(Map inputs) { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventEntity.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventEntity.java index 9659941fc9..3cfb67a7fc 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventEntity.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventEntity.java @@ -84,6 +84,10 @@ public class FileRunEventEntity implements Serializable { @Column(name = "run_id") private String runId; + /** Which folder, bucket or webhook fed the run. Null when a user supplied the file. */ + @Column(name = "source_id") + private String sourceId; + @Column(name = "file_id") private String fileId; diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java index 6da6a9b004..1bee2398f7 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java @@ -17,19 +17,21 @@ import org.springframework.transaction.annotation.Transactional; public interface FileRunEventRepository extends JpaRepository { /** - * This team's events, newest first, scoped in the query rather than loaded and filtered. A - * {@code null} teamId matches the rows with no team (login disabled), mirroring {@link - * stirling.software.proprietary.policy.source.SourceRepository#findByTeam}, since a plain - * {@code = null} would return nothing. + * As {@link #findByTeamAndStatus} but for a set of statuses, e.g. the open ones. The kind + * filter is in the query, before the limit: filtering an already-limited page could return + * nothing while matching rows exist. */ @Query( "select e from FileRunEventEntity e where ((:teamId is null and e.teamId is null) or" - + " e.teamId = :teamId) and (:kindId is null or e.kindId = :kindId)" - + " order by e.lastSeenAt desc") - List findByTeam( - @Param("teamId") Long teamId, @Param("kindId") String kindId, Pageable pageable); + + " e.teamId = :teamId) and e.status in :statuses" + + " and (:kindId is null or e.kindId = :kindId) order by e.lastSeenAt desc") + List findByTeamAndStatusIn( + @Param("teamId") Long teamId, + @Param("statuses") List statuses, + @Param("kindId") String kindId, + Pageable pageable); - /** As {@link #findByTeam} but restricted to one status, for the review surface's filters. */ + /** As {@link #findByTeamAndStatusIn} but for exactly one status, for the surface's filters. */ @Query( "select e from FileRunEventEntity e where ((:teamId is null and e.teamId is null) or" + " e.teamId = :teamId) and e.status = :status" @@ -85,6 +87,31 @@ public interface FileRunEventRepository extends JpaRepository allowedFrom); + /** + * Close the incidents about documents their owner deleted from the editor: the queue is what + * needs attention, and a document that no longer exists needs none. + * + *

Restricted to that owner's own editor rows. File ids are minted by the client, so scoping + * on team alone would let one caller close a colleague's incidents by naming ids. Processor + * rows are excluded outright: nothing was deleted from an editor there. + */ + @Modifying(clearAutomatically = true) + @Transactional + @Query( + "update FileRunEventEntity e set e.status =" + + " stirling.software.proprietary.failure.FileRunEventStatus.FILE_REMOVED," + + " e.statusActor = :actor, e.statusAt = :now where e.origin =" + + " stirling.software.proprietary.failure.FailureOrigin.TOOL and ((:teamId is" + + " null and e.teamId is null) or e.teamId = :teamId) and ((:actor is null and" + + " e.actor is null) or e.actor = :actor) and e.fileId in :fileIds and e.status in" + + " :allowedFrom") + int markFilesRemoved( + @Param("teamId") Long teamId, + @Param("actor") String actor, + @Param("fileIds") Collection fileIds, + @Param("now") Instant now, + @Param("allowedFrom") Collection allowedFrom); + /** * The most recent row for this exact failure, so the rollup can increment an existing incident * instead of opening a new one. Team-scoped, so the same failure in two teams stays two rows. diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java index 94b9c00a2c..57eb4698e4 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventService.java @@ -31,6 +31,67 @@ public class FileRunEventService { private final UserServiceInterface userService; private final ApplicationProperties applicationProperties; + /** + * Record a failure a user hit in the editor. One incident per named file, so each document + * stays separately actionable; one unattributed incident when the report names none. + * + *

The team and actor come from the session rather than the report, and the kind is + * classified from the reported code, falling back to {@link FailureKind#UNKNOWN} for a code no + * kind claims. + */ + public List report(EditorFailureReport report) { + FailureKind kind = FailureKind.byErrorCode(report.errorCode()).orElse(FailureKind.UNKNOWN); + Long teamId = scope().teamId(); + String actor = currentActor(); + String detail = detailFor(report); + + List fileIds = + report.fileIds().stream().filter(id -> id != null && !id.isBlank()).toList(); + if (fileIds.isEmpty()) { + return List.of(recordReported(kind, teamId, actor, null, detail)); + } + // Every named file gets its row. An earlier cap here silently dropped the rest, which lost + // failures a reviewer needed and was inconsistent with the processor path, where a sweep + // records one row per failing file with no limit at all. How many files one report may name + // is bounded at the boundary instead (see EditorFailureReport#MAX_FILE_IDS), where an + // oversized report can be refused whole before anything is written. + return fileIds.stream() + .map(fileId -> recordReported(kind, teamId, actor, fileId, detail)) + .toList(); + } + + private FileRunEvent recordReported( + FailureKind kind, Long teamId, String actor, String fileId, String detail) { + return store.record(RecordFailure.forEditor(kind, teamId, actor, fileId, detail)); + } + + /** + * The operation is the context a reviewer needs, since an editor failure has no policy or run. + */ + private String detailFor(EditorFailureReport report) { + String message = report.detail() == null ? "" : report.detail(); + return message.isBlank() ? report.operation() : report.operation() + ": " + message; + } + + /** + * Close the incidents about documents the caller has deleted from their editor. The queue means + * "needs attention", and a document that no longer exists needs none; the rows stay for audit. + * + *

Best-effort by nature: this only arrives if the browser that owns the file says so, and a + * cleared cache or another device never will. Rows left open that way are retention's problem, + * not this method's. + * + * @return how many incidents were closed + */ + public int forgetFiles(List fileIds) { + TeamScope scope = scope(); + if (!scope.permitted()) { + return 0; + } + List named = fileIds.stream().filter(id -> id != null && !id.isBlank()).toList(); + return store.markFilesRemoved(scope.teamId(), currentActor(), named); + } + /** The calling user's events, newest first. Empty when their team cannot be resolved. */ public List list(FileRunEventStatus status, String kindId, int limit) { TeamScope scope = scope(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java index cae0529334..9bf0e0b607 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStatus.java @@ -11,7 +11,14 @@ public enum FileRunEventStatus { NEW(false), ACKNOWLEDGED(false), DISMISSED(true), - RESOLVED(true); + RESOLVED(true), + + /** + * The document this incident was about was deleted from its owner's editor, so there is nothing + * left to act on. Distinct from {@code DISMISSED}, which is a reviewer's decision, and from + * {@code RESOLVED}, which reopens on recurrence: this one cannot recur, the file is gone. + */ + FILE_REMOVED(true); /** The statuses a review queue shows by default: everything still needing a decision. */ private static final List OPEN = diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java index e1fc7cc453..cf336ab76e 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java @@ -98,6 +98,7 @@ public class FileRunEventStore { entity.setOrigin(command.origin()); entity.setPolicyId(command.policyId()); entity.setRunId(command.runId()); + entity.setSourceId(command.sourceId()); entity.setFileId(command.fileId()); entity.setDetail(command.detail()); entity.setDedupKey(dedupKey); @@ -112,9 +113,14 @@ public class FileRunEventStore { } /** - * A page of incidents, newest first, optionally narrowed to one status and one kind. The kind - * filter lives in the query, before the limit: filtering a already-limited page could return - * nothing while matching rows exist. + * A page of incidents, newest first, optionally narrowed to one status and one kind. + * + *

With no status asked for this is the open queue rather than every row ever + * recorded: a dismissed failure has been dealt with, and leaving it in the default view means + * the list can never be cleared. Ask for a status to see closed rows. + * + *

Both filters live in the query, before the limit: filtering an already-limited page could + * return nothing while matching rows exist. */ @Transactional(readOnly = true) public List list( @@ -122,7 +128,8 @@ public class FileRunEventStore { Pageable page = PageRequest.of(0, Math.max(1, limit)); List rows = status == null - ? repository.findByTeam(teamId, kindId, page) + ? repository.findByTeamAndStatusIn( + teamId, FileRunEventStatus.open(), kindId, page) : repository.findByTeamAndStatus(teamId, status, kindId, page); return rows.stream().map(FileRunEvent::of).toList(); } @@ -184,6 +191,22 @@ public class FileRunEventStore { .orElseThrow(() -> refusalFor(id, teamId)); } + /** + * Close this owner's open incidents about {@code fileIds}, because the documents are gone. The + * rows stay for audit; they just leave the queue. Only open rows move, so a reviewer's dismiss + * keeps its meaning and its actor. + * + * @return how many incidents were closed + */ + @Transactional + public int markFilesRemoved(Long teamId, String actor, Collection fileIds) { + if (fileIds.isEmpty()) { + return 0; + } + return repository.markFilesRemoved( + teamId, actor, fileIds, Instant.now(), FileRunEventStatus.open()); + } + /** * Why the guarded UPDATE refused, worked out only once it has. Missing and closed are told * apart after the fact rather than before, so the answer describes the row the UPDATE saw. diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java index cbaab4684d..3809fad2c6 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventView.java @@ -22,6 +22,7 @@ public record FileRunEventView( String detail, String policyId, String runId, + String sourceId, String fileId, String actor, int occurrences, @@ -48,6 +49,7 @@ public record FileRunEventView( event.detail(), event.policyId(), event.runId(), + event.sourceId(), event.fileId(), event.actor(), event.occurrences(), diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/PolicyFailureRecorder.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/PolicyFailureRecorder.java index 7d4516939a..eaa8e0f5bd 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/PolicyFailureRecorder.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/PolicyFailureRecorder.java @@ -30,11 +30,12 @@ public class PolicyFailureRecorder { public void recordRunFailure( String runId, String policyId, - String actor, + String sourceId, String fileIdentity, + String actor, String detail, Throwable cause) { - record(classifier.classify(cause), runId, policyId, actor, fileIdentity, detail); + record(classifier.classify(cause), runId, policyId, sourceId, fileIdentity, actor, detail); } /** @@ -44,21 +45,35 @@ public class PolicyFailureRecorder { * pick the wrong one. */ public void recordRunFailureAs( - FailureKind kind, String runId, String policyId, String actor, String detail) { - record(kind, runId, policyId, actor, null, detail); + FailureKind kind, + String runId, + String policyId, + String sourceId, + String actor, + String detail) { + // No document reference: a run rejected at admission never got as far as one. + record(kind, runId, policyId, sourceId, null, actor, detail); } private void record( FailureKind kind, String runId, String policyId, - String actor, + String sourceId, String fileIdentity, + String actor, String detail) { try { store.record( RecordFailure.forRun( - kind, teamFor(policyId), actor, policyId, runId, fileIdentity, detail)); + kind, + teamFor(policyId), + actor, + policyId, + runId, + sourceId, + fileIdentity, + detail)); } catch (RuntimeException e) { // Deliberately swallowed: see the class comment. log.warn("Could not record failure event for run {} (kind {})", runId, kind.getId(), e); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/RecordFailure.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/RecordFailure.java index 1f45d52e17..8448f26fc3 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/RecordFailure.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/RecordFailure.java @@ -38,17 +38,40 @@ public record RecordFailure( detail = truncate(detail); } - /** A processor-side failure with no file or source context, e.g. a run that failed outright. */ + /** + * A processor-side run failure. {@code sourceId} says which folder, bucket or webhook fed the + * run, and is the only attribution an unattended failure has: there is no user to name. {@code + * fileId} is the source's opaque reference to the document, already hashed upstream. + */ public static RecordFailure forRun( FailureKind kind, Long teamId, String actor, String policyId, String runId, + String sourceId, String fileId, String detail) { return new RecordFailure( - kind, FailureOrigin.POLICY, teamId, actor, policyId, runId, null, fileId, detail); + kind, + FailureOrigin.POLICY, + teamId, + actor, + policyId, + runId, + sourceId, + fileId, + detail); + } + + /** + * A failure a user hit in their own editor. There is no policy, run or source: the user is the + * attribution, and {@code fileId} may be null when the report named no file. + */ + public static RecordFailure forEditor( + FailureKind kind, Long teamId, String actor, String fileId, String detail) { + return new RecordFailure( + kind, FailureOrigin.TOOL, teamId, actor, null, null, null, fileId, detail); } /** @@ -56,14 +79,21 @@ public record RecordFailure( * reference are the same incident; see {@link #dedupKey()}. */ public String scopeRef() { - return switch (kind.getScope()) { - case FILE -> nullToEmpty(policyId) + "|" + fileOrRun(); - case RUN -> nullToEmpty(runId); - case POLICY -> nullToEmpty(policyId); - case SOURCE -> nullToEmpty(sourceId); - // One server-wide condition is one incident regardless of which run tripped over it. - case SERVER -> ""; - }; + String about = + switch (kind.getScope()) { + case FILE -> nullToEmpty(policyId) + "|" + fileOrRun(); + // An editor report has no run, so a RUN-scoped kind would otherwise put every + // such failure in a team into one incident: fall back to the document. + case RUN -> isBlank(runId) ? fileOrRun() : nullToEmpty(runId); + case POLICY -> nullToEmpty(policyId); + case SOURCE -> nullToEmpty(sourceId); + // One server-wide condition is one incident regardless of which run hit it. + case SERVER -> ""; + }; + // An editor failure belongs to the person who hit it, so two colleagues hitting the same + // thing are two incidents. Folding them would credit one actor for both and offer the + // wrong person the row. Unattended runs have no such owner and are unaffected. + return origin == FailureOrigin.TOOL ? nullToEmpty(actor) + "|" + about : about; } /** diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java index a58670ac25..1a424176ce 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java @@ -130,25 +130,27 @@ public class PolicyEngine { // worker. String principal = currentActingPrincipal(); return submitForPrincipal( - principal, principal, policyId, definition, inputs, null, listener); + principal, principal, policyId, definition, inputs, listener, null, null); } /** Run a stored policy on demand. {@code enabled} gates triggers, not explicit runs. */ public PolicyRunHandle runPolicy( Policy policy, PolicyInputs inputs, PolicyProgressListener listener) { - return runPolicy(policy, inputs, null, listener); + return runPolicy(policy, inputs, listener, null, null); } /** - * As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, with the source's opaque - * reference to the document being run. Carried so a failure can say which document it was - * about, and so the same document failing again folds into one incident. + * As {@link #runPolicy(Policy, PolicyInputs, PolicyProgressListener)}, recording which source + * fed the run and its opaque reference to the document. The first says where an unattended + * failure came from; the second says which document, and is what lets the same document failing + * again fold into one incident. Both null for a user's upload. */ public PolicyRunHandle runPolicy( Policy policy, PolicyInputs inputs, - String fileIdentity, - PolicyProgressListener listener) { + PolicyProgressListener listener, + String sourceId, + String fileIdentity) { // Bill the policy owner: trigger-fired runs have no security context, and the async worker // doesn't inherit the caller's, so the owner (stamped at policy creation) is the reliable // billing identity — and for org-wide policies the org/owner is meant to pay. But own the @@ -172,9 +174,12 @@ public class PolicyEngine { fileOwner, policy.id(), definition, + // main's asset-resolved inputs, not the raw ones: stored certificates and watermark + // images bind here, before the async hop, because worker threads have no principal. resolved, - fileIdentity, - listener); + listener, + sourceId, + fileIdentity); } private PolicyRunHandle submitForPrincipal( @@ -183,8 +188,9 @@ public class PolicyEngine { String policyId, PipelineDefinition definition, PolicyInputs inputs, - String fileIdentity, - PolicyProgressListener listener) { + PolicyProgressListener listener, + String sourceId, + String fileIdentity) { // Scope the run id to the current user (this request thread) so the file-download // ownership check passes. No-op when security is off. String runId = jobOwnershipService.createScopedJobKey(UUID.randomUUID().toString()); @@ -193,7 +199,7 @@ public class PolicyEngine { if (policyId != null) { taskManager.putMetadata(runId, "policyId", policyId); } - PolicyRun run = new PolicyRun(runId, policyId, definition, fileIdentity); + PolicyRun run = new PolicyRun(runId, policyId, definition, sourceId, fileIdentity); registry.register(run); CompletableFuture completion = new CompletableFuture<>(); PolicyProgressListener tracking = trackingListener(runId, run, listener); @@ -359,7 +365,12 @@ public class PolicyEngine { // No exception to classify here: nothing was thrown by a tool, the run simply was not // admitted. Record it explicitly so a run lost to load pressure is still accounted for. failureRecorder.recordRunFailureAs( - FailureKind.UNKNOWN, run.getRunId(), run.getPolicyId(), null, message); + FailureKind.UNKNOWN, + run.getRunId(), + run.getPolicyId(), + run.getSourceId(), + null, + message); completion.complete(run); } return null; @@ -373,8 +384,9 @@ public class PolicyEngine { failureRecorder.recordRunFailure( run.getRunId(), run.getPolicyId(), - MDC.get(AUDIT_PRINCIPAL_MDC_KEY), + run.getSourceId(), run.getFileIdentity(), + MDC.get(AUDIT_PRINCIPAL_MDC_KEY), message, cause); } diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java index 04dda0871e..b4609f94ea 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyRunner.java @@ -79,7 +79,8 @@ public class PolicyRunner { // Generator pipeline: one run with no input. Still fall through to the cleanup // below so rows recorded for its folder outputs are pruned like anything else, // instead of accumulating until the policy is deleted. - runIds.add(startRun(policy, PolicyInputs.of(List.of()), null, unused -> {})); + // Generator pipeline: no input, so neither a source nor a document to attribute to. + runIds.add(startRun(policy, null, null, PolicyInputs.of(List.of()), unused -> {})); } for (PipelineInput input : inputs) { String sourceId = input.sourceId(); @@ -167,7 +168,13 @@ public class PolicyRunner { List runIds = new ArrayList<>(); long docsFed = 0; for (ResolvedInput unit : work) { - runIds.add(startRun(policy, unit.inputs(), unit.fileIdentity(), unit.onComplete())); + runIds.add( + startRun( + policy, + sourceId, + unit.fileIdentity(), + unit.inputs(), + unit.onComplete())); docsFed += unit.inputs().primary().size(); } docCounter.record(sourceId, docsFed); @@ -175,10 +182,15 @@ public class PolicyRunner { } private String startRun( - Policy policy, PolicyInputs inputs, String fileIdentity, Consumer onComplete) { + Policy policy, + String sourceId, + String fileIdentity, + PolicyInputs inputs, + Consumer onComplete) { log.info("Running policy {} ({})", policy.id(), policy.name()); PolicyRunHandle handle = - policyEngine.runPolicy(policy, inputs, fileIdentity, PolicyProgressListener.NOOP); + policyEngine.runPolicy( + policy, inputs, PolicyProgressListener.NOOP, sourceId, fileIdentity); handle.completion() .whenComplete((run, throwable) -> onComplete.accept(succeeded(run, throwable))); return handle.runId(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java index 24a97f6c2a..65cfa658ca 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java @@ -21,6 +21,12 @@ public class PolicyRun { /** ID of the stored policy that produced this run; null for ad-hoc pipelines. */ private final String policyId; + /** + * ID of the source the input came from (folder, S3, webhook), or null when a user supplied the + * files. Recorded on a failure so a reviewer can see where an unattended file came from. + */ + private final String sourceId; + private final PipelineDefinition definition; /** @@ -56,10 +62,21 @@ public class PolicyRun { private volatile List outputs = List.of(); private volatile Instant updatedAt = Instant.now(); + /** + * Both references are required rather than defaulted: a run with neither is a real case (a + * user's upload, an ad-hoc pipeline), but it should be stated at the call site. Overloads that + * omitted them would make losing the attribution the frictionless option, which is how both + * fields went unpopulated in the first place. + */ public PolicyRun( - String runId, String policyId, PipelineDefinition definition, String fileIdentity) { + String runId, + String policyId, + PipelineDefinition definition, + String sourceId, + String fileIdentity) { this.runId = runId; this.policyId = policyId; + this.sourceId = sourceId; this.definition = definition; this.fileIdentity = fileIdentity; } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java index 1a1b1a1599..a9baf5288f 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FailureKindTest.java @@ -182,10 +182,10 @@ class FailureKindTest { class Unknown { @Test - void existsAndCanBeTriaged() { - assertThat(FailureKind.UNKNOWN.getActions()) - .containsExactlyInAnyOrder( - FailureActionId.ACKNOWLEDGE, FailureActionId.DISMISS); + void offersOnlyTheActionThatClearsIt() { + // Nothing here can be fixed, so "seen it" and "clear it" would be the same decision. + // Offering both just asks the reviewer to press two buttons to reach one outcome. + assertThat(FailureKind.UNKNOWN.getActions()).containsExactly(FailureActionId.DISMISS); } @Test @@ -233,8 +233,14 @@ class FailureKindTest { @Test void declaresOnlyWhatItLists() { - assertThat(FailureKind.UNKNOWN.declares(FailureActionId.ACKNOWLEDGE)).isTrue(); assertThat(FailureKind.UNKNOWN.declares(FailureActionId.DISMISS)).isTrue(); + assertThat(FailureKind.UNKNOWN.declares(FailureActionId.ACKNOWLEDGE)).isFalse(); + } + + @Test + void aKindWithSomethingToFixOffersTheFixAndAWayToSkipIt() { + assertThat(FailureKind.INPUT_PASSWORD_PROTECTED.getActions()) + .containsExactly(FailureActionId.ACKNOWLEDGE, FailureActionId.DISMISS); } @Test diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java index a4b830d0f8..088cffe8ec 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java @@ -74,6 +74,20 @@ class FileRunEventControllerTest { "the raw failure message")); } + private static List fileIds(int count) { + return java.util.stream.IntStream.range(0, count).mapToObj(i -> "f-" + i).toList(); + } + + /** The status a refused call came back with. Fails the test if the call was allowed. */ + private HttpStatus statusOf(Runnable call) { + try { + call.run(); + } catch (ResponseStatusException e) { + return HttpStatus.valueOf(e.getStatusCode().value()); + } + throw new AssertionError("expected the call to be refused"); + } + @Nested @DisplayName("listing") class Listing { @@ -117,6 +131,7 @@ class FileRunEventControllerTest { @Test void showsAClosedRowsActionsDisabledWithAReasonRatherThanHidingThem() { + // Only visible by asking for the closed status: the default queue drops it. FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); controller.act(event.id(), "DISMISS", null); @@ -134,15 +149,16 @@ class FileRunEventControllerTest { @Test void filtersByStatusAndByKind() { - FileRunEvent open = given(FailureKind.UNKNOWN, TEAM, "open"); - given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked"); - controller.act(open.id(), "ACKNOWLEDGE", null); + FileRunEvent locked = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked"); + given(FailureKind.UNKNOWN, TEAM, "open"); + controller.act(locked.id(), "ACKNOWLEDGE", null); assertThat(controller.list(FileRunEventStatus.ACKNOWLEDGED, null, null).events()) .hasSize(1); assertThat(controller.list(null, "INPUT_PASSWORD_PROTECTED", null).events()) .extracting(FileRunEventView::fileId) .containsExactly("locked"); + // Acknowledged is still open work, so it stays in the default queue. assertThat(controller.list(null, "NO_SUCH_KIND", null).events()).isEmpty(); } @@ -182,7 +198,7 @@ class FileRunEventControllerTest { @Test void appliesADeclaredActionAndReturnsTheUpdatedRow() { - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); FileRunEventView updated = controller.act(event.id(), "ACKNOWLEDGE", null); @@ -218,21 +234,12 @@ class FileRunEventControllerTest { @Test void anAlreadyClosedRowIsAConflict() { // The request was well formed and would have been valid a moment earlier. - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); controller.act(event.id(), "DISMISS", null); assertThat(statusOf(() -> controller.act(event.id(), "ACKNOWLEDGE", null))) .isEqualTo(HttpStatus.CONFLICT); } - - private HttpStatus statusOf(Runnable call) { - try { - call.run(); - } catch (ResponseStatusException e) { - return HttpStatus.valueOf(e.getStatusCode().value()); - } - throw new AssertionError("expected the call to be refused"); - } } @Nested @@ -387,6 +394,108 @@ class FileRunEventControllerTest { } } + @Nested + @DisplayName("reporting from the editor") + class Reporting { + + @Test + void aMemberMayReportEvenThoughTheyMayNotRead() { + // The asymmetry is the point: anyone whose work failed can say so, but only a leader + // reviews the queue. + when(authority.canEditPolicies()).thenReturn(false); + + assertThatCode( + () -> + controller.report( + new EditorFailureReport( + "compress", "E004", List.of("f-1"), "boom"))) + .doesNotThrowAnyException(); + assertThatThrownBy(() -> controller.list(null, null, null)) + .isInstanceOf(ResponseStatusException.class); + } + + @Test + void answersWithNoContentSoTheEditorNeverWaitsOnABody() { + EditorFailureReport report = + new EditorFailureReport("compress", "E004", List.of("f-1"), "boom"); + + assertThat(controller.report(report).getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT); + } + + @Test + void rejectsAReportWithNoOperation() { + assertThat( + statusOf( + () -> + controller.report( + new EditorFailureReport( + " ", "E004", List.of("f-1"), "boom")))) + .isEqualTo(HttpStatus.BAD_REQUEST); + } + + @Test + void acceptsAReportAtTheFileLimitAndRecordsEveryRow() { + List atLimit = fileIds(EditorFailureReport.MAX_FILE_IDS); + + EditorFailureReport report = + new EditorFailureReport("compress", "E004", atLimit, "boom"); + + assertThat(controller.report(report).getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT); + assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10)) + .hasSize(EditorFailureReport.MAX_FILE_IDS); + } + + @Test + void refusesAReportOverTheFileLimitAndRecordsNothing() { + // One call used to be able to mint an unbounded number of permanent incidents, since + // each named file gets its own row and TOOL dedup keys never fold across ids. Refused + // rather than trimmed so nothing is lost silently, and refused before the first write + // so a rejected report cannot leave a partial set behind either. + List overLimit = fileIds(EditorFailureReport.MAX_FILE_IDS + 1); + + assertThat( + statusOf( + () -> + controller.report( + new EditorFailureReport( + "compress", + "E004", + overLimit, + "boom")))) + .isEqualTo(HttpStatus.BAD_REQUEST); + assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10)) + .isEmpty(); + } + + @Test + void saysWhatTheLimitIsSoAClientAuthorCanSeeWhatHappened() { + // The editor reports in the background, so the message is the only place this surfaces. + assertThatThrownBy( + () -> + controller.report( + new EditorFailureReport( + "compress", + "E004", + fileIds(EditorFailureReport.MAX_FILE_IDS + 1), + "boom"))) + .isInstanceOf(ResponseStatusException.class) + .hasMessageContaining(String.valueOf(EditorFailureReport.MAX_FILE_IDS)); + } + + @Test + void theReportHasNoTeamOrFileNameToSupply() { + // Stated as a test because the absence of those fields is the property. Adding either + // to + // EditorFailureReport breaks this at compile time. + List components = + java.util.Arrays.stream(EditorFailureReport.class.getRecordComponents()) + .map(java.lang.reflect.RecordComponent::getName) + .toList(); + + assertThat(components).containsExactly("operation", "errorCode", "fileIds", "detail"); + } + } + @Nested @DisplayName("action request body") class RequestBody { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java index 3537c9e06c..b3ee4b7d15 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java @@ -152,9 +152,9 @@ class FileRunEventHttpIntegrationTest { @Test void coercesQueryParametersAndFiltersOnThem() throws Exception { - String open = seed(FailureKind.UNKNOWN, TEAM, "open", "a"); - seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked", "b"); - post("/api/v1/file-run-events/" + open + "/actions/ACKNOWLEDGE", "{\"inputs\":{}}"); + String locked = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "locked", "b"); + seed(FailureKind.UNKNOWN, TEAM, "open", "a"); + post("/api/v1/file-run-events/" + locked + "/actions/ACKNOWLEDGE", "{\"inputs\":{}}"); JsonNode acknowledged = mapper.readTree(get("/api/v1/file-run-events?status=ACKNOWLEDGED").body()) @@ -189,6 +189,75 @@ class FileRunEventHttpIntegrationTest { } } + @Nested + @DisplayName("reporting from the editor") + class Reporting { + + @Test + void bindsAReportBodyAndAnswersNoContent() throws Exception { + HttpResponse response = + post( + "/api/v1/file-run-events/reports", + "{\"operation\":\"remove-password\",\"errorCode\":\"E004\"," + + "\"fileIds\":[\"f-1\",\"f-2\"],\"detail\":\"locked\"}"); + + assertThat(response.statusCode()).isEqualTo(204); + assertThat(response.body()).isEmpty(); + + JsonNode events = mapper.readTree(get("/api/v1/file-run-events").body()).get("events"); + assertThat(events).hasSize(2); + assertThat(events.get(0).get("origin").asString()).isEqualTo("TOOL"); + assertThat(events.get(0).get("kindId").asString()) + .isEqualTo("INPUT_PASSWORD_PROTECTED"); + } + + @Test + void acceptsAReportWithNoCodeOrFiles() throws Exception { + HttpResponse response = + post( + "/api/v1/file-run-events/reports", + "{\"operation\":\"compress\",\"detail\":\"network died\"}"); + + assertThat(response.statusCode()).isEqualTo(204); + JsonNode events = mapper.readTree(get("/api/v1/file-run-events").body()).get("events"); + assertThat(events).hasSize(1); + assertThat(events.get(0).get("kindId").asString()).isEqualTo("UNKNOWN"); + assertThat(events.get(0).get("fileId").isNull()).isTrue(); + } + + @Test + void rejectsAReportWithNoOperation() throws Exception { + assertThat( + post( + "/api/v1/file-run-events/reports", + "{\"errorCode\":\"E004\",\"detail\":\"boom\"}") + .statusCode()) + .isEqualTo(400); + } + + @Test + void rejectsAnOversizedReportWithoutRecordingAnyOfIt() throws Exception { + // Over the wire because that is where the flood would arrive: one request, an + // arbitrarily long fileIds array, a permanent row per entry. The read-back is the point + // of the test, since a partial write would be worse than either accepting or refusing. + String ids = + java.util.stream.IntStream.range(0, EditorFailureReport.MAX_FILE_IDS + 1) + .mapToObj(i -> "\"f-" + i + "\"") + .collect(java.util.stream.Collectors.joining(",")); + + HttpResponse response = + post( + "/api/v1/file-run-events/reports", + "{\"operation\":\"compress\",\"errorCode\":\"E004\",\"fileIds\":[" + + ids + + "],\"detail\":\"boom\"}"); + + assertThat(response.statusCode()).isEqualTo(400); + assertThat(mapper.readTree(get("/api/v1/file-run-events").body()).get("events")) + .isEmpty(); + } + } + @Nested @DisplayName("action dispatch") class Dispatch { @@ -197,7 +266,7 @@ class FileRunEventHttpIntegrationTest { void bindsTheRequestBodyAndReturnsTheUpdatedRow() throws Exception { // The regression guard: an object body, sent as real JSON over the wire, binding into // ActionRequest. A double-encoded string would fail here. - String id = seed(FailureKind.UNKNOWN, TEAM, "f1", "boom"); + String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "boom"); HttpResponse response = post( @@ -262,7 +331,7 @@ class FileRunEventHttpIntegrationTest { @Test void mapsAnAlreadyClosedRowToConflict() throws Exception { - String id = seed(FailureKind.UNKNOWN, TEAM, "f1", "boom"); + String id = seed(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1", "boom"); post("/api/v1/file-run-events/" + id + "/actions/DISMISS", "{\"inputs\":{}}"); assertThat( diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java index cd75c181c5..fbbe8e6359 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java @@ -76,7 +76,7 @@ class FileRunEventServiceTest { @Test void movesANewEventToAcknowledgedAndStampsTheActor() { - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); FileRunEvent updated = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); @@ -87,7 +87,7 @@ class FileRunEventServiceTest { @Test void isANoOpWhenAlreadyAcknowledgedSoOwnershipIsNotStolen() { - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); FileRunEvent first = service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); Instant originalAt = first.statusAt(); @@ -114,7 +114,7 @@ class FileRunEventServiceTest { @Test void closesAnAcknowledgedEvent() { - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); service.dispatch(event.id(), "ACKNOWLEDGE", Map.of()); assertThat(service.dispatch(event.id(), "DISMISS", Map.of()).status()) @@ -225,7 +225,7 @@ class FileRunEventServiceTest { @Test void aClosedEventCannotBeActedOnAgain() { - FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f1"); + FileRunEvent event = given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f1"); service.dispatch(event.id(), "DISMISS", Map.of()); assertThatThrownBy(() -> service.dispatch(event.id(), "ACKNOWLEDGE", Map.of())) @@ -286,8 +286,7 @@ class FileRunEventServiceTest { assertThat(service.availableActions(event)) .extracting(FileRunEventService.AvailableAction::labelKey) - .containsExactlyInAnyOrder( - "portal.failures.action.acknowledge", "portal.failures.action.dismiss"); + .containsExactly("portal.failures.action.dismiss"); } } @@ -390,4 +389,253 @@ class FileRunEventServiceTest { } } } + + @Nested + @DisplayName("reporting a failure the user hit in the editor") + class Reporting { + + @Test + void classifiesTheReportedCodeAndStampsItAsEditorOrigin() { + service.report( + new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom")); + + FileRunEvent event = store.list(TEAM, null, null, 10).getFirst(); + assertThat(event.kind()).isEqualTo(FailureKind.INPUT_PASSWORD_PROTECTED); + assertThat(event.origin()).isEqualTo(FailureOrigin.TOOL); + assertThat(event.fileId()).isEqualTo("f-1"); + assertThat(event.detail()).contains("boom"); + } + + @Test + void takesTheTeamAndActorFromThePrincipalNotTheReport() { + // The report carries no team or actor field at all; both come from the caller's + // session. + service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom")); + + FileRunEvent event = store.list(TEAM, null, null, 10).getFirst(); + assertThat(event.teamId()).isEqualTo(TEAM); + assertThat(event.actor()).isEqualTo(ACTOR); + } + + @Test + void recordsAnUnrecognisedCodeAsUnknownRatherThanDroppingIt() { + service.report(new EditorFailureReport("ocr", "E999", List.of("f-1"), "no idea")); + + assertThat(store.list(TEAM, null, null, 10).getFirst().kind()) + .isEqualTo(FailureKind.UNKNOWN); + } + + @Test + void recordsAnAbsentCodeAsUnknown() { + service.report(new EditorFailureReport("ocr", null, List.of("f-1"), "network died")); + + assertThat(store.list(TEAM, null, null, 10).getFirst().kind()) + .isEqualTo(FailureKind.UNKNOWN); + } + + @Test + void recordsOneIncidentPerFileSoEachDocumentStaysActionable() { + service.report( + new EditorFailureReport( + "compress", "E004", List.of("f-1", "f-2", "f-3"), "boom")); + + assertThat(store.list(TEAM, null, null, 10)) + .hasSize(3) + .extracting(FileRunEvent::fileId) + .containsExactlyInAnyOrder("f-1", "f-2", "f-3"); + } + + @Test + void foldsARepeatOfTheSameFileIntoTheExistingIncident() { + service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom")); + service.report( + new EditorFailureReport("compress", "E004", List.of("f-1"), "boom again")); + + assertThat(store.list(TEAM, null, null, 10)) + .singleElement() + .extracting(FileRunEvent::occurrences) + .isEqualTo(2); + } + + @Test + void recordsOneUnattributedIncidentWhenNoFileWasNamed() { + service.report(new EditorFailureReport("compress", "E004", List.of(), "boom")); + + assertThat(store.list(TEAM, null, null, 10)) + .singleElement() + .extracting(FileRunEvent::fileId) + .isNull(); + } + + @Test + void recordsEveryFileInALargeBatchRatherThanTrimmingIt() { + // A cap here used to drop the overflow silently, so a reviewer saw 25 of 60 failures + // with nothing indicating the rest existed. The processor path has never had one. + List many = + java.util.stream.IntStream.range(0, 60).mapToObj(i -> "f-" + i).toList(); + + service.report(new EditorFailureReport("compress", "E004", many, "boom")); + + assertThat(store.list(TEAM, null, null, 200)).hasSize(60); + } + + @Test + void keepsTheOperationNameOutOfTheStoredDocumentReferences() { + // The operation is context for the reviewer, not a document reference: it belongs in + // detail, never in fileId. + service.report( + new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom")); + + FileRunEvent event = store.list(TEAM, null, null, 10).getFirst(); + assertThat(event.detail()).contains("remove-password"); + assertThat(event.fileId()).isEqualTo("f-1"); + } + + @Test + void storesTheReportedMessageVerbatimAlongsideTheOperation() { + // The user's own error about their own file. The operation is prefixed because an + // editor failure has no run to give a reviewer context. + service.report( + new EditorFailureReport( + "compress", "E004", List.of("f-1"), "Failed on Q4 report.pdf")); + + assertThat(store.list(TEAM, null, null, 10).getFirst().detail()) + .isEqualTo("compress: Failed on Q4 report.pdf"); + } + + @Test + void theServiceHoldsNothingThatCouldReachADocument() { + // Asserted structurally rather than with verifyNoInteractions on unwired mocks, which + // is how the version of this test on the other branch passed without proving anything. + List> forbidden = + List.of( + stirling.software.proprietary.policy.ledger.ProcessedLedger.class, + stirling.software.common.service.FileStorage.class, + stirling.software.proprietary.policy.output.PolicyOutputSink.class); + + List> held = + Arrays.stream(FileRunEventService.class.getDeclaredFields()) + .filter(field -> !field.isSynthetic()) + .map(Field::getType) + .toList(); + + assertThat(held).isNotEmpty().doesNotContainAnyElementsOf(forbidden); + } + } + + @Nested + @DisplayName("editor incidents stay separate") + class EditorIncidentIdentity { + + private void reportedBy(String actor, String fileId) { + when(userService.getCurrentUsername()).thenReturn(actor); + service.report(new EditorFailureReport("compress", null, List.of(fileId), "boom")); + } + + @Test + void twoPeopleHittingTheSameUnclassifiedFailureAreTwoIncidents() { + // UNKNOWN is RUN scoped and an editor report has no run, so without the fallback every + // unclassified editor failure in a team collapsed into one row: one actor credited for + // everyone's, and the wrong person offered the row. + reportedBy("alice@example.com", "a-1"); + reportedBy("bob@example.com", "b-1"); + + assertThat(store.list(TEAM, null, null, 10)) + .extracting(FileRunEvent::actor) + .containsExactlyInAnyOrder("alice@example.com", "bob@example.com"); + } + + @Test + void onePersonsTwoBrokenFilesAreTwoIncidents() { + reportedBy("alice@example.com", "a-1"); + reportedBy("alice@example.com", "a-2"); + + assertThat(store.list(TEAM, null, null, 10)) + .extracting(FileRunEvent::fileId) + .containsExactlyInAnyOrder("a-1", "a-2"); + } + + @Test + void theSamePersonHittingTheSameFileTwiceIsOneIncident() { + reportedBy("alice@example.com", "a-1"); + reportedBy("alice@example.com", "a-1"); + + assertThat(store.list(TEAM, null, null, 10)) + .singleElement() + .extracting(FileRunEvent::occurrences) + .isEqualTo(2); + } + } + + @Nested + @DisplayName("files deleted from the editor") + class RemovedFiles { + + private void reported(String fileId) { + service.report(new EditorFailureReport("compress", "E004", List.of(fileId), "boom")); + } + + @Test + void closeTheirIncidentsSoTheQueueStopsAskingAboutThem() { + reported("f-1"); + + assertThat(service.forgetFiles(List.of("f-1"))).isEqualTo(1); + assertThat(service.list(null, null, 10)) + .as("the open queue is what the reviewer works from") + .isEmpty(); + } + + @Test + void theRowSurvivesForAudit() { + reported("f-1"); + service.forgetFiles(List.of("f-1")); + + assertThat(service.list(FileRunEventStatus.FILE_REMOVED, null, 10)) + .singleElement() + .satisfies( + event -> { + assertThat(event.fileId()).isEqualTo("f-1"); + assertThat(event.detail()).contains("boom"); + }); + } + + @Test + void aReviewersDismissKeepsItsMeaningAndItsActor() { + reported("f-1"); + FileRunEvent event = service.list(null, null, 10).getFirst(); + service.dispatch(event.id(), "DISMISS", Map.of()); + + assertThat(service.forgetFiles(List.of("f-1"))) + .as("only open rows move; a closed one has already been decided") + .isZero(); + assertThat(service.list(FileRunEventStatus.DISMISSED, null, 10)).hasSize(1); + } + + @Test + void aColleaguesIncidentIsUntouched() { + // File ids come from the client, so naming one must not close someone else's row. + store.record( + RecordFailure.forEditor( + FailureKind.UNKNOWN, TEAM, "employee@example.com", "f-1", "theirs")); + + assertThat(service.forgetFiles(List.of("f-1"))).isZero(); + } + + @Test + void aProcessorIncidentIsUntouchedEvenOnTheSameFileId() { + // Nothing was deleted from an editor there, and the file may still be in the bucket. + given(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "f-1"); + + assertThat(service.forgetFiles(List.of("f-1"))).isZero(); + } + + @Test + void namingNoFilesClosesNothing() { + reported("f-1"); + + assertThat(service.forgetFiles(List.of())).isZero(); + assertThat(service.forgetFiles(java.util.Arrays.asList(null, " "))).isZero(); + assertThat(service.list(null, null, 10)).hasSize(1); + } + } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java index 46d2be8a0d..949904dfb9 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java @@ -6,6 +6,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Proxy; import java.time.Instant; +import java.util.List; import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; @@ -198,12 +199,66 @@ class FileRunEventStoreDbTest { FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, null, + null, "policy-1", runId, fileId, "locked"); } + @Test + @DisplayName("closing deleted files touches only that owner's own open editor rows") + void markFilesRemovedIsScopedBySqlNotByTheCaller() { + // The scoping is entirely in the JPQL, so the in-memory fake proves nothing about it: + // it implements the same rules by hand and would agree with a wrong query. + FileRunEvent mine = + store.record( + RecordFailure.forEditor( + FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom")); + FileRunEvent theirs = + store.record( + RecordFailure.forEditor( + FailureKind.UNKNOWN, TEAM, "colleague@example.com", "f-1", "boom")); + FileRunEvent otherTeam = + store.record( + RecordFailure.forEditor( + FailureKind.UNKNOWN, + OTHER_TEAM, + "owner@example.com", + "f-1", + "boom")); + FileRunEvent fromProcessor = store.record(failure(FailureKind.UNKNOWN, TEAM, "f-1")); + + int closed = store.markFilesRemoved(TEAM, "owner@example.com", List.of("f-1")); + + assertThat(closed).isEqualTo(1); + assertThat(store.find(mine.id(), TEAM).orElseThrow().status()) + .isEqualTo(FileRunEventStatus.FILE_REMOVED); + assertThat(store.find(theirs.id(), TEAM).orElseThrow().status()) + .as("another person's incident about their own file") + .isEqualTo(FileRunEventStatus.NEW); + assertThat(store.find(otherTeam.id(), OTHER_TEAM).orElseThrow().status()) + .as("another team entirely") + .isEqualTo(FileRunEventStatus.NEW); + assertThat(store.find(fromProcessor.id(), TEAM).orElseThrow().status()) + .as("nothing was deleted from an editor here") + .isEqualTo(FileRunEventStatus.NEW); + } + + @Test + @DisplayName("a row already closed by a reviewer is left as they left it") + void markFilesRemovedLeavesClosedRowsAlone() { + FileRunEvent event = + store.record( + RecordFailure.forEditor( + FailureKind.UNKNOWN, TEAM, "owner@example.com", "f-1", "boom")); + store.applyStatus(event.id(), TEAM, FileRunEventStatus.DISMISSED, "reviewer@example.com"); + + assertThat(store.markFilesRemoved(TEAM, "owner@example.com", List.of("f-1"))).isZero(); + assertThat(store.find(event.id(), TEAM).orElseThrow().statusActor()) + .isEqualTo("reviewer@example.com"); + } + @Test @DisplayName("the kind filter applies before the limit, not after") void kindFilterAppliesBeforeTheLimit() { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java index 2adbc7d2b2..714d65c29c 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java @@ -298,6 +298,25 @@ class FileRunEventStoreTest { .containsExactly("unteamed"); } + @Test + void dismissingARowClearsItFromTheDefaultQueue() { + // The reviewer's whole complaint: without this, dismissing changes the buttons and + // leaves the row sitting there, so the list can only ever grow. + FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom")); + store.applyStatus(event.id(), TEAM, FileRunEventStatus.DISMISSED, "reviewer"); + + assertThat(store.list(TEAM, null, null, 10)).isEmpty(); + assertThat(store.list(TEAM, FileRunEventStatus.DISMISSED, null, 10)).hasSize(1); + } + + @Test + void anAcknowledgedRowIsStillOpenWorkSoItStays() { + FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom")); + store.applyStatus(event.id(), TEAM, FileRunEventStatus.ACKNOWLEDGED, "reviewer"); + + assertThat(store.list(TEAM, null, null, 10)).hasSize(1); + } + @Test void filtersByStatus() { FileRunEvent open = store.record(failure(FailureKind.UNKNOWN, TEAM, "open", "a")); diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java index ed8b6f29cd..1873f6265b 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java @@ -6,6 +6,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.function.Function; @@ -53,16 +54,6 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository { return kindId == null || kindId.equals(entity.getKindId()); } - @Override - public List findByTeam(Long teamId, String kindId, Pageable pageable) { - return page( - newestFirst( - rows.values().stream() - .filter(e -> sameTeam(e, teamId) && sameKind(e, kindId)) - .toList()), - pageable); - } - @Override public List findByTeamAndStatus( Long teamId, FileRunEventStatus status, String kindId, Pageable pageable) { @@ -124,6 +115,46 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository { return 1; } + @Override + public int markFilesRemoved( + Long teamId, + String actor, + Collection fileIds, + Instant now, + Collection allowedFrom) { + int closed = 0; + for (FileRunEventEntity entity : rows.values()) { + if (entity.getOrigin() != FailureOrigin.TOOL + || !sameTeam(entity, teamId) + || !Objects.equals(entity.getActor(), actor) + || entity.getFileId() == null + || !fileIds.contains(entity.getFileId()) + || !allowedFrom.contains(entity.getStatus())) { + continue; + } + entity.setStatus(FileRunEventStatus.FILE_REMOVED); + entity.setStatusActor(actor); + entity.setStatusAt(now); + closed++; + } + return closed; + } + + @Override + public List findByTeamAndStatusIn( + Long teamId, List statuses, String kindId, Pageable pageable) { + return page( + newestFirst( + rows.values().stream() + .filter( + e -> + sameTeam(e, teamId) + && statuses.contains(e.getStatus()) + && sameKind(e, kindId)) + .toList()), + pageable); + } + @Override public List findByTeamAndDedupKey( Long teamId, String dedupKey, Pageable pageable) { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java index 841fdec684..e1a17843f0 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/PolicyFailureRecorderTest.java @@ -89,8 +89,9 @@ class PolicyFailureRecorderTest { recorder.recordRunFailure( "run-1", "policy-1", - "dana@example.com", null, + null, + "dana@example.com", "Policy run failed: locked", passwordFailure()); @@ -114,8 +115,9 @@ class PolicyFailureRecorderTest { recorder.recordRunFailure( "run-1", "policy-1", - "dana@example.com", null, + null, + "dana@example.com", "Policy run failed: something we do not recognise", new RuntimeException("boom")); @@ -133,6 +135,7 @@ class PolicyFailureRecorderTest { "policy-1", null, null, + null, "Policy run failed: java.lang.NullPointerException", new RuntimeException("npe")); @@ -147,7 +150,7 @@ class PolicyFailureRecorderTest { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailureAs( - FailureKind.UNKNOWN, "run-3", "policy-1", null, "could not be queued"); + FailureKind.UNKNOWN, "run-3", "policy-1", null, null, "could not be queued"); assertThat(store.list(TEAM, null, null, 10)).hasSize(1); } @@ -161,9 +164,21 @@ class PolicyFailureRecorderTest { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( - "run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure()); + "run-1", + "policy-1", + null, + null, + "dana@example.com", + "locked", + passwordFailure()); recorder.recordRunFailure( - "run-2", "policy-1", "dana@example.com", null, "locked", passwordFailure()); + "run-2", + "policy-1", + null, + null, + "dana@example.com", + "locked", + passwordFailure()); List events = store.list(TEAM, null, null, 10); assertThat(events).hasSize(2); @@ -173,14 +188,62 @@ class PolicyFailureRecorderTest { .containsExactlyInAnyOrder("run-1", "run-2"); } + @Test + void namesTheSourceWhenNoUserWasInvolved() { + // An unattended file has no actor, so the source is the only attribution a reviewer + // gets: which bucket, folder or webhook fed the run. + when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); + + recorder.recordRunFailure( + "run-1", + "policy-1", + "src-s3-invoices", + null, + null, + "locked", + passwordFailure()); + + FileRunEvent event = store.list(TEAM, null, null, 10).getFirst(); + assertThat(event.sourceId()).isEqualTo("src-s3-invoices"); + assertThat(event.actor()).isNull(); + } + + @Test + void keepsTwoSourcesApartEvenWhenTheyFailIdentically() { + // Same kind, same policy, no user on either: without the source they would be one row. + when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); + + recorder.recordRunFailureAs( + FailureKind.UNKNOWN, "run-1", "policy-1", "src-a", null, "unreachable"); + recorder.recordRunFailureAs( + FailureKind.UNKNOWN, "run-2", "policy-1", "src-b", null, "unreachable"); + + assertThat(store.list(TEAM, null, null, 10)) + .hasSize(2) + .extracting(FileRunEvent::sourceId) + .containsExactlyInAnyOrder("src-a", "src-b"); + } + @Test void thatSameRunFailingTwiceStaysOneIncident() { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( - "run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure()); + "run-1", + "policy-1", + null, + null, + "dana@example.com", + "locked", + passwordFailure()); recorder.recordRunFailure( - "run-1", "policy-1", "dana@example.com", null, "locked", passwordFailure()); + "run-1", + "policy-1", + null, + null, + "dana@example.com", + "locked", + passwordFailure()); assertThat(store.list(TEAM, null, null, 10)) .singleElement() @@ -198,7 +261,7 @@ class PolicyFailureRecorderTest { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( - "run-1", "policy-1", null, null, "boom", new RuntimeException()); + "run-1", "policy-1", null, null, null, "boom", new RuntimeException()); assertThat(store.list(TEAM, null, null, 10)).hasSize(1); } @@ -207,7 +270,8 @@ class PolicyFailureRecorderTest { void leavesAnAdHocRunUnteamedRatherThanGuessing() { // No stored policy means no team to attribute it to. Recorded unteamed rather than // attributed to whichever team happened to be nearby. - recorder.recordRunFailure("run-1", null, null, null, "boom", new RuntimeException()); + recorder.recordRunFailure( + "run-1", null, null, null, null, "boom", new RuntimeException()); assertThat(store.list(null, null, null, 10)).hasSize(1); assertThat(store.list(TEAM, null, null, 10)).isEmpty(); @@ -224,6 +288,7 @@ class PolicyFailureRecorderTest { "policy-1", null, null, + null, "boom", new RuntimeException())) .doesNotThrowAnyException(); @@ -258,6 +323,7 @@ class PolicyFailureRecorderTest { "policy-1", null, null, + null, "Policy run failed: locked", passwordFailure())) .doesNotThrowAnyException(); @@ -270,7 +336,13 @@ class PolicyFailureRecorderTest { assertThatCode( () -> recorder.recordRunFailure( - "run-1", "policy-1", null, null, "no cause", null)) + "run-1", + "policy-1", + null, + null, + null, + "no cause", + null)) .doesNotThrowAnyException(); assertThat(store.list(TEAM, null, null, 10).getFirst().kind()) .isEqualTo(FailureKind.UNKNOWN); @@ -286,9 +358,9 @@ class PolicyFailureRecorderTest { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( - "run-1", "policy-1", null, null, "boom", new IOException("x")); + "run-1", "policy-1", null, null, null, "boom", new IOException("x")); recorder.recordRunFailure( - "run-1", "policy-1", null, null, "boom", new IOException("x")); + "run-1", "policy-1", null, null, null, "boom", new IOException("x")); List events = store.list(TEAM, null, null, 10); assertThat(events).hasSize(1); @@ -301,9 +373,9 @@ class PolicyFailureRecorderTest { when(policyStore.get("policy-1")).thenReturn(Optional.of(policy("policy-1", TEAM))); recorder.recordRunFailure( - "run-1", "policy-1", null, null, "boom", new IOException("x")); + "run-1", "policy-1", null, null, null, "boom", new IOException("x")); recorder.recordRunFailure( - "run-2", "policy-1", null, null, "boom", new IOException("x")); + "run-2", "policy-1", null, null, null, "boom", new IOException("x")); assertThat(store.list(TEAM, null, null, 10)).hasSize(2); } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java index ef9648571f..86c52c4007 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/RecordFailurePrivacyTest.java @@ -24,7 +24,14 @@ class RecordFailurePrivacyTest { private static RecordFailure withDetail(String detail) { return RecordFailure.forRun( - FailureKind.UNKNOWN, 1L, "dana@example.com", "policy-1", "run-1", null, detail); + FailureKind.UNKNOWN, + 1L, + "dana@example.com", + "policy-1", + "run-1", + null, + null, + detail); } @Test diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java index 43477aa9ed..c38f5c7d89 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/controller/PolicyControllerTest.java @@ -216,7 +216,7 @@ class PolicyControllerTest { } private static PolicyRunHandle handle(String runId) { - PolicyRun run = new PolicyRun(runId, null, definitionWithStep(), null); + PolicyRun run = new PolicyRun(runId, null, definitionWithStep(), null, null); return new PolicyRunHandle(runId, CompletableFuture.completedFuture(run)); } @@ -317,7 +317,7 @@ class PolicyControllerTest { @Test @DisplayName("returns the run view when present") void found() { - PolicyRun run = new PolicyRun("run-3", null, definitionWithStep(), null); + PolicyRun run = new PolicyRun("run-3", null, definitionWithStep(), null, null); when(runRegistry.get("run-3")).thenReturn(run); ResponseEntity response = controller.status("run-3"); @@ -346,9 +346,11 @@ class PolicyControllerTest { @Test @DisplayName("excludes ad-hoc runs and runs owned by others") void filtersRuns() { - PolicyRun adHoc = new PolicyRun("adhoc", null, definitionWithStep(), null); - PolicyRun ownedStored = new PolicyRun("owned", "policy-A", definitionWithStep(), null); - PolicyRun otherStored = new PolicyRun("other", "policy-B", definitionWithStep(), null); + PolicyRun adHoc = new PolicyRun("adhoc", null, definitionWithStep(), null, null); + PolicyRun ownedStored = + new PolicyRun("owned", "policy-A", definitionWithStep(), null, null); + PolicyRun otherStored = + new PolicyRun("other", "policy-B", definitionWithStep(), null, null); when(runRegistry.all()).thenReturn(List.of(adHoc, ownedStored, otherStored)); // ownedByCurrentUser: strip then re-apply scope reproduces the key only for the owned diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyEngineTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyEngineTest.java index b3aaf291f9..b3b45dcd96 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyEngineTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyEngineTest.java @@ -225,7 +225,41 @@ class PolicyEngineTest { // A failed run is recorded durably, so an admin can see it after the in-memory run expires. verify(failureRecorder) .recordRunFailure( - eq(runId), any(), any(), any(), anyString(), any(Throwable.class)); + eq(runId), any(), any(), any(), any(), anyString(), any(Throwable.class)); + } + + @Test + void recordsWhichSourceFedAFailedRun() throws Exception { + // The source is threaded onto the run so an unattended failure is attributable: there is no + // user to name for a file that arrived from a bucket. + when(toolMetadataService.isMultiInput(ROTATE)).thenReturn(false); + when(internalApiClient.post(eq(ROTATE), any())).thenThrow(new RuntimeException("boom")); + + PolicyRunHandle handle = + engine.runPolicy( + new Policy( + "p1", + "rotate", + "owner", + true, + List.of(), + List.of(new PipelineStep(ROTATE, Map.of())), + OutputSpec.inline()), + PolicyInputs.of(List.of(pdf("input", "input.pdf"))), + PolicyProgressListener.NOOP, + "src-s3-invoices", + "file-hash-1"); + handle.completion().get(10, TimeUnit.SECONDS); + + verify(failureRecorder) + .recordRunFailure( + anyString(), + any(), + eq("src-s3-invoices"), + eq("file-hash-1"), + any(), + anyString(), + any(Throwable.class)); } @Test @@ -238,7 +272,7 @@ class PolicyEngineTest { doThrow(new RuntimeException("event store unavailable")) .when(failureRecorder) .recordRunFailure( - anyString(), any(), any(), any(), anyString(), any(Throwable.class)); + anyString(), any(), any(), any(), any(), anyString(), any(Throwable.class)); PolicyRunHandle handle = engine.submit( diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunRegistryTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunRegistryTest.java index 784baa207c..1eb0a1a1fe 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunRegistryTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunRegistryTest.java @@ -97,7 +97,11 @@ class PolicyRunRegistryTest { private PolicyRun register(String runId) { PolicyRun run = new PolicyRun( - runId, null, new PipelineDefinition(runId, List.of(), List.of()), null); + runId, + null, + new PipelineDefinition(runId, List.of(), List.of()), + null, + null); registry.register(run); return run; } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java index ba66cb8b2d..d6f5951e3a 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/engine/PolicyRunnerTest.java @@ -78,13 +78,13 @@ class PolicyRunnerTest { @Test void runsOnceWithNoFilesWhenThePolicyHasNoSources() { Policy policy = policy(List.of()); - when(policyEngine.runPolicy(eq(policy), any(), any(), any())) + when(policyEngine.runPolicy(eq(policy), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); runner.run(policy); ArgumentCaptor inputs = ArgumentCaptor.forClass(PolicyInputs.class); - verify(policyEngine).runPolicy(eq(policy), inputs.capture(), any(), any()); + verify(policyEngine).runPolicy(eq(policy), inputs.capture(), any(), any(), any()); assertTrue(inputs.getValue().primary().isEmpty()); // Ledger hygiene still runs: rows recorded for a generator policy's folder outputs // are pruned by its own sweeps rather than accumulating until the policy is deleted. @@ -137,12 +137,12 @@ class PolicyRunnerTest { List.of( ResolvedInput.of(PolicyInputs.of(List.of())), ResolvedInput.of(PolicyInputs.of(List.of())))); - when(policyEngine.runPolicy(any(), any(), any(), any())) + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); runner.run(policy); - verify(policyEngine, times(2)).runPolicy(eq(policy), any(), any(), any()); + verify(policyEngine, times(2)).runPolicy(eq(policy), any(), any(), any(), any()); } @Test @@ -154,7 +154,7 @@ class PolicyRunnerTest { when(folderSource.supports(spec)).thenReturn(true); when(folderSource.resolve(eq(spec), any())).thenReturn(List.of(unit)); CompletableFuture completion = new CompletableFuture<>(); - when(policyEngine.runPolicy(any(), any(), any(), any())) + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", completion)); runner.run(policy); @@ -175,7 +175,7 @@ class PolicyRunnerTest { when(folderSource.supports(spec)).thenReturn(true); when(folderSource.resolve(eq(spec), any())).thenReturn(List.of(unit)); CompletableFuture completion = new CompletableFuture<>(); - when(policyEngine.runPolicy(any(), any(), any(), any())) + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", completion)); runner.run(policy); @@ -224,12 +224,12 @@ class PolicyRunnerTest { when(folderSource.supports(spec)).thenReturn(true); when(folderSource.resolve(eq(spec), any())) .thenReturn(List.of(ResolvedInput.of(PolicyInputs.of(List.of())))); - when(policyEngine.runPolicy(any(), any(), any(), any())) + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); runner.run(policy, SweepKind.LIGHT); - verify(policyEngine).runPolicy(eq(policy), any(), any(), any()); + verify(policyEngine).runPolicy(eq(policy), any(), any(), any(), any()); verify(processedLedger, never()).markSeen(any(), any()); verify(processedLedger, never()).deleteUnseen(any(), anyLong()); } @@ -244,12 +244,13 @@ class PolicyRunnerTest { when(folderSource.resolve(eq(broken), any())).thenThrow(new IOException("mount gone")); when(folderSource.resolve(eq(healthy), any())) .thenReturn(List.of(ResolvedInput.of(PolicyInputs.of(List.of())))); - when(policyEngine.runPolicy(any(), any(), any(), any())) + when(policyEngine.runPolicy(any(), any(), any(), any(), any())) .thenReturn(new PolicyRunHandle("r", new CompletableFuture<>())); runner.run(policy); - verify(policyEngine).runPolicy(eq(policy), any(), any(), any()); // healthy source still ran + verify(policyEngine) + .runPolicy(eq(policy), any(), any(), any(), any()); // healthy source still ran verify(processedLedger, never()).deleteUnseen(any(), anyLong()); // history preserved } diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 4ddc2497ee..d85158dcb7 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -7332,9 +7332,11 @@ retry = "Try again" title = "Something went wrong on this page" [portal.failures] +fromSource = "From source {{source}}" occurrences = "{{count}} occurrences" +reportedBy = "Hit by {{actor}}" runReference = "Run {{runId}}" -subtitle = "Failures recorded from your policy runs, with the actions you can take." +subtitle = "Failures recorded from your policy runs and your team's editors, with the actions you can take." title = "Failures" [portal.failures.action] @@ -7359,6 +7361,11 @@ title = "Password-protected document" description = "This run failed for a reason Stirling does not yet recognise. The raw message is shown below." title = "Unrecognised failure" +[portal.failures.origin] +pipeline = "Pipeline" +policy = "Policy" +tool = "Tool run" + [portal.failures.stage] blocked = "Blocked" input = "Input" diff --git a/frontend/editor/src/core/contexts/FileContext.tsx b/frontend/editor/src/core/contexts/FileContext.tsx index c627d61f35..634562375c 100644 --- a/frontend/editor/src/core/contexts/FileContext.tsx +++ b/frontend/editor/src/core/contexts/FileContext.tsx @@ -67,6 +67,7 @@ import { alert } from "@app/components/toast"; import { buildRemovePasswordFormData } from "@app/hooks/tools/removePassword/buildRemovePasswordFormData"; import type { RemovePasswordParameters } from "@app/hooks/tools/removePassword/useRemovePasswordParameters"; import apiClient from "@app/services/apiClient"; +import { reportFilesRemoved } from "@app/services/failureReporting"; import { processResponse } from "@app/utils/toolResponseProcessor"; import { ToolOperation } from "@app/types/file"; import { handlePasswordError } from "@app/utils/toolErrorHandler"; @@ -610,6 +611,10 @@ function FileContextInner({ // Remove from memory and cleanup resources lifecycleManager.removeFiles(fileIds, stateRef); + // Any failure recorded against these stops needing attention: the document is gone. + // Fire-and-forget, so a server that cannot be told never blocks the delete. + void reportFilesRemoved(fileIds); + // Remove from IndexedDB if enabled if (indexedDB && enablePersistence && deleteFromStorage !== false) { try { diff --git a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts index f01c8770ab..fe1ddc7bc7 100644 --- a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts @@ -21,6 +21,7 @@ import { StirlingFileStub, } from "@app/types/fileContext"; import { FILE_EVENTS } from "@app/services/errorUtils"; +import { reportToolFailure } from "@app/services/failureReporting"; import { zipFileService } from "@app/services/zipFileService"; import { getFilenameWithoutExtension } from "@app/utils/fileUtils"; import { @@ -603,6 +604,14 @@ export const useToolOperation = ( void _e; } + // Report it so a leader sees the failure too, then carry on with the user's + // own error handling. Fire-and-forget: the reporter swallows its own errors. + void reportToolFailure({ + operation: config.operationType, + error, + fileIds: validFiles.map((file) => file.fileId), + }); + const errorMessage = config.getErrorMessage?.(error) || extractErrorMessage(error); actions.setError(errorMessage); diff --git a/frontend/editor/src/core/services/failureReporting.test.ts b/frontend/editor/src/core/services/failureReporting.test.ts new file mode 100644 index 0000000000..2eadc3ae65 --- /dev/null +++ b/frontend/editor/src/core/services/failureReporting.test.ts @@ -0,0 +1,315 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Tests for the editor's failure reporter. Two properties matter: it never sends a + * document name, and it never lets its own failure reach the tool the user was + * running. + */ + +const post = vi.fn(); + +/** + * Indirection so one test can replace the transport with a plain throwing closure. + * A vi.fn that throws has its error re-reported by vitest even once the code under + * test has caught it, which would fail the very test asserting it was caught. + */ +let transport: (...args: unknown[]) => unknown = (...args) => post(...args); + +vi.mock("@app/services/apiClient", () => ({ + default: { post: (...args: unknown[]) => transport(...args) }, +})); + +const { reportToolFailure, reportFilesRemoved, errorCodeOf } = + await import("@app/services/failureReporting"); + +/** An axios-shaped rejection carrying a Problem Details body. */ +function problemDetail(errorCode: string, extra: Record = {}) { + return { + response: { + status: 400, + data: { type: "/errors/pdf-password", errorCode, ...extra }, + }, + message: "Request failed with status code 400", + }; +} + +describe("errorCodeOf", () => { + it("reads the code out of a Problem Details body", async () => { + await expect(errorCodeOf(problemDetail("E004"))).resolves.toBe("E004"); + }); + + it("reads the code out of a blob body, which is how a download-typed call fails", async () => { + const error = { + response: { + data: { + text: () => Promise.resolve(JSON.stringify({ errorCode: "E001" })), + }, + }, + }; + + await expect(errorCodeOf(error)).resolves.toBe("E001"); + }); + + it("returns null when the body carries no code", async () => { + await expect( + errorCodeOf({ response: { data: { title: "nope" } } }), + ).resolves.toBeNull(); + await expect(errorCodeOf({ message: "network error" })).resolves.toBeNull(); + await expect(errorCodeOf(undefined)).resolves.toBeNull(); + }); + + it("returns null rather than throwing on an unparseable blob", async () => { + const error = { + response: { data: { text: () => Promise.resolve("502") } }, + }; + + await expect(errorCodeOf(error)).resolves.toBeNull(); + }); +}); + +describe("reportToolFailure", () => { + beforeEach(() => { + post.mockReset().mockResolvedValue({ status: 204 }); + transport = (...args) => post(...args); + }); + + it("posts the operation, code and file ids", async () => { + await reportToolFailure({ + operation: "remove-password", + error: problemDetail("E004"), + fileIds: ["f-1", "f-2"], + }); + + expect(post).toHaveBeenCalledTimes(1); + const [path, body] = post.mock.calls[0] as [ + string, + Record, + ]; + expect(path).toBe("/api/v1/file-run-events/reports"); + expect(body).toMatchObject({ + operation: "remove-password", + errorCode: "E004", + fileIds: ["f-1", "f-2"], + }); + }); + + it("ignores names a caller hands it, and identifies files by id", async () => { + // fileNames is accepted and dropped on purpose, so a call site holding names cannot pass + // them somewhere they would be stored as a document reference. + await reportToolFailure({ + operation: "compress", + error: { response: { status: 500, data: {} }, message: "boom" }, + fileIds: ["f-1"], + fileNames: ["Q4 report.pdf"], + }); + + const body = post.mock.calls[0]?.[1] as Record; + expect(body.fileIds).toEqual(["f-1"]); + expect(JSON.stringify(body)).not.toContain("Q4 report.pdf"); + }); + + it("sends the message the user saw, unaltered", async () => { + // Their own error about their own file: trimming it only makes the row harder to act on. + await reportToolFailure({ + operation: "compress", + error: { + response: { status: 500, data: {} }, + message: "Failed on Q4 report.pdf", + }, + fileIds: ["f-1"], + }); + + const body = post.mock.calls[0]?.[1] as { detail: string }; + expect(body.detail).toBe("Failed on Q4 report.pdf"); + }); + + it("sends no team, because the server derives it", async () => { + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: ["f-1"], + }); + + expect(JSON.stringify(post.mock.calls[0]?.[1])).not.toMatch(/team/i); + }); + + it("swallows its own failure so the tool's own error handling is unaffected", async () => { + transport = () => { + throw new Error("404 - no such route on a core build"); + }; + + let threw = false; + try { + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: ["f-1"], + }); + } catch { + threw = true; + } + + expect(threw).toBe(false); + }); + + it("asks for its own failure not to be shown, since the tool's error is already on screen", async () => { + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: ["f-1"], + }); + + expect(post.mock.calls[0]?.[2]).toMatchObject({ suppressErrorToast: true }); + }); + + it("logs a rejected report instead of losing it, and still does not throw", async () => { + // A 400 means this client built a bad report, e.g. one naming more files than a report may + // carry. Nothing else would ever surface it: the call is fire-and-forget and its toast is + // suppressed, so the console line is the only sign a client author gets. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + transport = () => { + throw Object.assign(new Error("Request failed with status code 400"), { + response: { + status: 400, + data: { + detail: + "a report may name at most 200 files, and this one named 5000", + }, + }, + }); + }; + + let threw = false; + try { + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: ["f-1"], + }); + } catch { + threw = true; + } + + expect(threw).toBe(false); + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain("at most 200 files"); + warn.mockRestore(); + }); + + it("stays quiet when the route is simply absent, as on a build without failure tracking", async () => { + // Otherwise every tool failure on such a build would log, which is noise rather than a + // diagnostic. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + transport = () => { + throw Object.assign(new Error("Request failed with status code 404"), { + response: { status: 404, data: {} }, + }); + }; + + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: ["f-1"], + }); + + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + it("reports a client-side refusal, which is a failure a leader can act on", async () => { + // The same class of problem as the processor rejecting a file type, which is + // already recorded. Unclassified, so the server files it as UNKNOWN. + await reportToolFailure({ + operation: "convert", + error: new Error("Unsupported conversion format"), + fileIds: ["f-1"], + }); + + const body = post.mock.calls[0]?.[1] as Record; + expect(body).toMatchObject({ + operation: "convert", + errorCode: null, + detail: "Unsupported conversion format", + }); + }); + + it("reports a network failure, which got no reply but did leave the browser", async () => { + await reportToolFailure({ + operation: "compress", + error: { request: {}, message: "Network Error" }, + fileIds: ["f-1"], + }); + + expect(post).toHaveBeenCalledTimes(1); + }); + + it.each([ + ["an axios cancellation", { code: "ERR_CANCELED", message: "canceled" }], + [ + "the rethrown wrapper useToolApiCalls builds", + new Error("Operation was cancelled", { + cause: { code: "ERR_CANCELED" }, + }), + ], + ])("ignores %s, because the user chose to stop", async (_label, error) => { + await reportToolFailure({ operation: "compress", error, fileIds: ["f-1"] }); + + expect(post).not.toHaveBeenCalled(); + }); + + it("tells the server when files are deleted, so their failures leave the queue", async () => { + await reportFilesRemoved(["f-1", "f-2"]); + + const [path, body] = post.mock.calls[0] as [ + string, + Record, + ]; + expect(path).toBe("/api/v1/file-run-events/removed-files"); + expect(body).toEqual({ fileIds: ["f-1", "f-2"] }); + }); + + it("says nothing when no real file ids were deleted", async () => { + await reportFilesRemoved([]); + await reportFilesRemoved(["", " "]); + + expect(post).not.toHaveBeenCalled(); + }); + + it("swallows a failed deletion notice, since the file is gone locally either way", async () => { + transport = () => { + throw new Error("404 - no such route on a core build"); + }; + + let threw = false; + try { + await reportFilesRemoved(["f-1"]); + } catch { + threw = true; + } + + expect(threw).toBe(false); + }); + + it("does nothing without an operation to attribute the failure to", async () => { + await reportToolFailure({ + operation: "", + error: { response: { status: 500 } }, + fileIds: ["f-1"], + }); + + expect(post).not.toHaveBeenCalled(); + }); + + it("sends every file id, since trimming would lose failures silently", async () => { + const many = Array.from({ length: 60 }, (_, i) => `f-${i}`); + + await reportToolFailure({ + operation: "compress", + error: problemDetail("E004"), + fileIds: many, + }); + + const body = post.mock.calls[0]?.[1] as { fileIds: string[] }; + expect(body.fileIds).toEqual(many); + }); +}); diff --git a/frontend/editor/src/core/services/failureReporting.ts b/frontend/editor/src/core/services/failureReporting.ts new file mode 100644 index 0000000000..af0765f8bd --- /dev/null +++ b/frontend/editor/src/core/services/failureReporting.ts @@ -0,0 +1,183 @@ +import apiClient from "@app/services/apiClient"; + +/** + * Reports a tool failure the user hit in the editor, so it lands in the same queue + * as one from a folder or bucket. The editor calls tools directly, so nothing + * server-side knows about these unless the client says so. + * + * Best-effort throughout: a build without the failure registry has no such route, + * and a report failing must never disturb the tool's own error handling. + */ + +const REPORT_PATH = "/api/v1/file-run-events/reports"; +const REMOVED_FILES_PATH = "/api/v1/file-run-events/removed-files"; + +interface ToolFailureReport { + /** The tool that failed, e.g. `remove-password`. */ + operation: string; + /** Whatever the tool call rejected with. */ + error: unknown; + /** Opaque file ids from FileContext. Names are deliberately not accepted. */ + fileIds?: string[]; + /** + * Accepted and ignored, so a call site that has names on hand cannot pass them + * somewhere they would be stored. Present to make that explicit rather than to + * be used. + */ + fileNames?: string[]; +} + +/** + * The `errorCode` from a tool's Problem Details response, or null when there is + * none. A download-typed call fails with a Blob body, so that shape is parsed too. + */ +export async function errorCodeOf(error: unknown): Promise { + const data = (error as { response?: { data?: unknown } })?.response?.data; + if (!data) return null; + + const body = await asJson(data); + const code = (body as { errorCode?: unknown })?.errorCode; + return typeof code === "string" && code.trim() !== "" ? code : null; +} + +async function asJson(data: unknown): Promise { + if (typeof data === "object" && data !== null && !isBlobLike(data)) { + return data; + } + try { + const text = isBlobLike(data) + ? await data.text() + : typeof data === "string" + ? data + : ""; + return text ? JSON.parse(text) : null; + } catch { + return null; + } +} + +function isBlobLike(value: unknown): value is { text: () => Promise } { + return ( + typeof value === "object" && + value !== null && + typeof (value as { text?: unknown }).text === "function" + ); +} + +export async function reportToolFailure({ + operation, + error, + fileIds = [], +}: ToolFailureReport): Promise { + if (!operation || operation.trim() === "") return; + if (wasCancelled(error)) return; + + try { + await apiClient.post( + REPORT_PATH, + { + operation, + errorCode: await errorCodeOf(error), + fileIds, + detail: messageOf(error), + }, + // The reporter's own failure must not reach the user: they already have the + // tool's error on screen, and a second toast about the report would be noise + // about something they never asked for. + { suppressErrorToast: true }, + ); + } catch (reportError) { + // Still never rethrown: a core build has no such route, and a member's report can + // also be refused. But a report the server rejected as invalid is logged rather + // than lost, because nothing else would ever surface it. + warnIfRejected(operation, reportError); + } +} + +/** + * A report the server refused as malformed, which means this client built a bad one: worth a + * line in the console for whoever wrote it, since the call is fire-and-forget and its toast is + * suppressed. An absent route (404, a core build) or a session not allowed to report are + * expected, and stay quiet so an ordinary build does not log on every tool failure. + */ +function warnIfRejected(operation: string, error: unknown): void { + const response = (error as { response?: { status?: number; data?: unknown } }) + ?.response; + if (response?.status !== 400) return; + + console.warn( + `Failure report for "${operation}" was rejected by the server: ${reasonOf(response.data)}`, + ); +} + +/** Whatever the server said, out of a Problem Details body. */ +function reasonOf(data: unknown): string { + const body = data as { detail?: unknown; message?: unknown }; + const stated = + typeof body?.detail === "string" + ? body.detail + : typeof body?.message === "string" + ? body.message + : ""; + return stated.trim() === "" ? "no reason given" : stated; +} + +/** + * Tell the server a user deleted these files, so any failure recorded against them stops asking + * for attention. The rows stay for audit; they just leave the queue. + * + *

Best-effort like the reporter: a build without the failure registry has no such route, and + * deleting a file must not fail because the server could not be told. + */ +export async function reportFilesRemoved(fileIds: string[]): Promise { + const named = fileIds.filter( + (id) => typeof id === "string" && id.trim() !== "", + ); + if (named.length === 0) return; + + try { + // Toast suppressed for the same reason as a report: the user deleted a file and is not + // waiting to hear whether the server was told. + await apiClient.post( + REMOVED_FILES_PATH, + { fileIds: named }, + { suppressErrorToast: true }, + ); + } catch { + // The file is gone locally either way. A row left open is retention's problem. + } +} + +/** + * The message the user saw, sent as-is. It is their own error about their own file, so hiding + * parts of it would only make the row harder to act on. + */ +function messageOf(error: unknown): string { + const candidate = error as { message?: unknown }; + return typeof candidate?.message === "string" ? candidate.message : ""; +} + +/** + * A user cancelling is the one failure worth dropping: nothing went wrong and there + * is nothing for a reviewer to do. `useToolApiCalls` rethrows an axios cancellation + * as a plain Error with the original as its cause, so both shapes are checked. + * + *

Everything else is reported, client-side refusals included: an unsupported input + * format is the same class of problem as the processor rejecting a file type, which + * is already recorded. + */ +function wasCancelled(error: unknown): boolean { + const candidate = error as { + code?: unknown; + name?: unknown; + message?: unknown; + cause?: { code?: unknown; name?: unknown }; + }; + return ( + candidate?.code === "ERR_CANCELED" || + candidate?.cause?.code === "ERR_CANCELED" || + candidate?.name === "CanceledError" || + candidate?.cause?.name === "CanceledError" || + candidate?.message === "Operation was cancelled" + ); +} diff --git a/frontend/editor/src/core/tests/convert/ConvertIntegration.test.tsx b/frontend/editor/src/core/tests/convert/ConvertIntegration.test.tsx index 42c61cf5dc..5f40cb87ae 100644 --- a/frontend/editor/src/core/tests/convert/ConvertIntegration.test.tsx +++ b/frontend/editor/src/core/tests/convert/ConvertIntegration.test.tsx @@ -569,8 +569,14 @@ describe("Convert Tool Integration Tests", () => { await result.current.executeOperation(parameters, [testFile]); }); - // Verify integration: utils validation prevents API call, hook shows error - expect(mockedApiClient.post).not.toHaveBeenCalled(); + // Verify integration: utils validation prevents the conversion call, hook shows + // error. Failure reporting posts separately and is not a conversion request. + const conversionCalls = vi + .mocked(mockedApiClient.post) + .mock.calls.filter( + ([url]) => !String(url).includes("/file-run-events/"), + ); + expect(conversionCalls).toHaveLength(0); expect(result.current.errorMessage).toContain( "Unsupported conversion format", ); diff --git a/frontend/editor/src/portal/api/fileRunEvents.ts b/frontend/editor/src/portal/api/fileRunEvents.ts index 8b52c7d3fc..768618132e 100644 --- a/frontend/editor/src/portal/api/fileRunEvents.ts +++ b/frontend/editor/src/portal/api/fileRunEvents.ts @@ -35,7 +35,9 @@ export type FileRunEventStatus = | "NEW" | "ACKNOWLEDGED" | "DISMISSED" - | "RESOLVED"; + | "RESOLVED" + /** Its document was deleted from the owner's editor, so there is nothing left to act on. */ + | "FILE_REMOVED"; /** * One button as offered for one row. `id` is a plain string rather than a union @@ -64,6 +66,8 @@ export interface FileRunEvent { detail: string | null; policyId: string | null; runId: string | null; + /** Which folder, bucket or webhook fed the run. Null when a user supplied the file. */ + sourceId: string | null; /** * Opaque reference, never a name. Only the owner's own client can resolve it to * something readable, from its local file store. diff --git a/frontend/editor/src/portal/components/failures/FailureActionButtons.test.tsx b/frontend/editor/src/portal/components/failures/FailureActionButtons.test.tsx index 7982eb15ce..2409a03ec9 100644 --- a/frontend/editor/src/portal/components/failures/FailureActionButtons.test.tsx +++ b/frontend/editor/src/portal/components/failures/FailureActionButtons.test.tsx @@ -70,6 +70,7 @@ function event(actions: FailureActionOffer[]): FileRunEvent { detail: "boom", policyId: "p1", runId: "r1", + sourceId: null, fileId: "f-1", actor: "someone@example.com", occurrences: 1, diff --git a/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx b/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx index 1458a1d64d..9ec3af6842 100644 --- a/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx +++ b/frontend/editor/src/portal/components/failures/FileRunEventList.test.tsx @@ -21,7 +21,14 @@ vi.mock("react-i18next", () => ({ useTranslation: () => ({ // Faithful to i18next: a known key resolves, an unknown key falls back to // defaultValue. That is what exercises the server-key-then-generic chain. - t: (key: string, options?: { defaultValue?: string } | string) => { + // i18next's real signature: t(key, options) or t(key, defaultValue, options). + t: ( + key: string, + second?: { defaultValue?: string } | string, + third?: Record, + ) => { + const options = typeof second === "string" ? third : second; + const fallback = typeof second === "string" ? second : undefined; const known: Record = { "portal.failures.kind.inputPasswordProtected.title": "Password-protected document", @@ -30,11 +37,20 @@ vi.mock("react-i18next", () => ({ "portal.failures.occurrences": "occurrences", "portal.failures.runReference": "Run r1", "portal.failures.stage.input": "Input", + "portal.failures.origin.tool": "Tool run", + "portal.failures.origin.policy": "Policy", }; + if (key === "portal.failures.fromSource") { + return `From source ${(options as { source?: string })?.source ?? ""}`; + } + if (key === "portal.failures.reportedBy") { + return `Hit by ${(options as { actor?: string })?.actor ?? ""}`; + } if (known[key]) return known[key]; - if (typeof options === "string") return options; - if (options?.defaultValue) return options.defaultValue; - return key; + if ((options as { defaultValue?: string })?.defaultValue) { + return (options as { defaultValue: string }).defaultValue; + } + return fallback ?? key; }, }), })); @@ -61,6 +77,7 @@ function event(overrides: Partial = {}): FileRunEvent { detail: "The PDF Document is passworded", policyId: "p1", runId: "r1", + sourceId: null, fileId: "f-1", actor: "dana@example.com", occurrences: 1, @@ -102,6 +119,29 @@ describe("FileRunEventList", () => { expect(screen.getByText("The PDF Document is passworded")).toBeTruthy(); }); + it("names the person whose editor hit it, and marks it a tool run", async () => { + // The point of reporting editor failures: a reviewer needs the person, since a + // run reference means nothing for a failure that never had a run. + fetchFileRunEvents.mockResolvedValue([ + event({ origin: "TOOL", actor: "dana@example.com", runId: null }), + ]); + + render(); + + expect(await screen.findByText("Tool run")).toBeTruthy(); + expect(screen.getByText("Hit by dana@example.com")).toBeTruthy(); + }); + + it("names the source when no user was involved, since that is the only attribution", async () => { + fetchFileRunEvents.mockResolvedValue([ + event({ origin: "POLICY", actor: null, sourceId: "src-s3-invoices" }), + ]); + + render(); + + expect(await screen.findByText("From source src-s3-invoices")).toBeTruthy(); + }); + it("shows the occurrence count only once a failure has repeated", async () => { fetchFileRunEvents.mockResolvedValue([event({ occurrences: 1 })]); const { unmount } = render(); diff --git a/frontend/editor/src/portal/components/failures/FileRunEventList.tsx b/frontend/editor/src/portal/components/failures/FileRunEventList.tsx index 9f75c5bbd9..477af32e50 100644 --- a/frontend/editor/src/portal/components/failures/FileRunEventList.tsx +++ b/frontend/editor/src/portal/components/failures/FileRunEventList.tsx @@ -29,6 +29,7 @@ export function FileRunEventList() { const { apply, refresh } = useFileRunEventActions(); const [busy, setBusy] = useState<{ id: string; action: string } | null>(null); const [showJson, setShowJson] = useState(false); + const [clearing, setClearing] = useState(false); // A build without the proprietary module has no such route, and a caller who is // not a team leader gets a 403. Both mean there is nothing to show. @@ -43,6 +44,26 @@ export function FileRunEventList() { } }; + // Empties the queue so a test run starts from nothing. Sequential rather than + // concurrent: dismissing is cheap, and one request at a time keeps the failure + // obvious if the endpoint refuses one of them. + const dismissAll = async () => { + setClearing(true); + try { + for (const event of events ?? []) { + const dismiss = event.actions.find( + (action) => action.id === "DISMISS" && action.enabled, + ); + if (dismiss) { + await apply(event.id, "DISMISS"); + } + } + } finally { + setClearing(false); + await refresh(); + } + }; + // Dev-only inspector for hand-checking classification against real uploads. // Vite folds `import.meta.env.DEV` to false, so builds drop this entirely. const debugPanel = !import.meta.env.DEV ? null : ( @@ -50,6 +71,14 @@ export function FileRunEventList() { +