OTTER-524: surface why a job errored, and let the reviewer close out a run with no outputs

[therealmarv]

see OTTER-524

Companion PR: safeinsights/iac#221. Either side can merge first. See "Deploy order" below.

What was reported

A study ran against a code environment with a wrong image URL. The run errored. The reviewer's page showed no error log. Only the security scan log appeared. At the same time, the copy told the reviewer to review error logs.

What the job actually did

I checked the reported job in staging CodeBuild before I made any change. Job 019db1e5-bd39-729b-8246-63b2668215d2:

• the source scan succeeded. That scan wrote the security scan log.
• the image packaging build failed. The build never reached the enclave. The code never ran.
• the build log holds the real cause: UNAUTHORIZED: project opensta not found. The URL said opensta, not openstax.
• the build then logged the webhook body that it sent to us: {"jobId":"019db1e5-...","status":"JOB-ERRORED"}

The missing error log is not a display bug. The build sent nothing. No log exists anywhere else. AWS writes no container log when a task does not start. The packaging step sends no log.

Three causes

1. A packaging failure could not report a reason. This app builds the failure payload. The payload held only the job id and the status. The webhook schema had no field for a reason. The schema strips unknown keys, so a reason could not arrive.
2. The screen asked the wrong question. It asked "does this job have any log?". A security scan log is a log. The screen must ask "does this job have something that explains a failed run?". Three services write into the same encrypted-log set: the code scanner at submission, the containerizer on a packaging failure, and the enclave when a container exits non-zero.
3. A run with no artifacts was a dead end. The reviewer had nothing to decrypt. The screen showed "No encrypted outputs available to decrypt". The reviewer never reached the decision section, so no FILES-* decision was possible. The researcher then stayed on "code is running" forever, because that screen ends only when a decision exists.

What this change does

Name the stage that failed

src/lib/job-error-details.ts reads the failed stage from the status history. This works when the build sends nothing:

• JOB-PACKAGING but no JOB-READY: packaging failed. The image could not be prepared, so the code never ran.
• JOB-READY but no JOB-RUNNING: the job was packaged, but it never started.
• JOB-RUNNING: the code ran and did not finish.
• none of these: the run did not complete, and the screen names no stage. /api/services/job-scan-results and /api/job/[jobId] can both record JOB-ERRORED before the containerizer starts. That reviewer must not read that the image could not be prepared.

Report the reason for the failure

The companion iac PR wraps one build step: the crane config call. That step fails when the image URL is wrong, when we cannot read the image, or when someone deleted the image after approval. The step then adds one key to the failure payload:

{ "jobId": "...", "status": "JOB-ERRORED", "failureReason": "BASE_IMAGE_UNAVAILABLE" }

This app maps that code to a sentence. That sentence replaces the stage sentence.

Three rules keep infrastructure detail out of this channel. Each rule has a test.

• A code, never text. A reviewer works for a data partner, not for us. Build text is not safe to show to them. Command lines hold the S3 bucket name and the S3 path layout. AWS stderr can hold the account id and IAM role names. The build log starts with the webhook endpoint and the length of the webhook secret. Therefore the build sends a code and nothing else. Sentry still receives the full error, so we lose nothing for debugging.
• Classify, never echo. jobErrorDetails compares the stored value against a known list. It then shows our own sentence. An unknown value falls back to the stage sentence. This rule also protects us from the enclave, which writes a raw AWS error into the same column today.
• A query that cannot fail the page. latestRecordedJobFailureReason filters on the known code set in SQL. An empty set would render message in (), which Postgres rejects, so the query returns null before it builds that statement. If someone retires the last code, the reviewer reads the stage sentence. The screen does not fail.
• Read only for a reviewer. latestRecordedJobFailureReason is a separate small query. The webhook route stores only known codes, and the query selects only known codes. Raw service text therefore never leaves the database. latestJobForStudyQuery and getStudyJobInfo do not select message. Both of those feed actions that the submitting researcher can call.

The build classifies only that one step. An S3 failure or a Harbor failure is our problem, not the data partner's. Those failures keep the bare payload, and the reviewer keeps the general sentence.

Use one rule for the banner and the key form

The errored screen asks one question. Does this job hold an encrypted artifact about the run's own outcome? That artifact is an ENCRYPTED-RESULT or an encrypted error log. jobHasDecryptableRunOutcome answers the question. The same module builds the banner sentence from the same file list. The screen therefore cannot say one thing and do another.

Artifacts on the job	Key form	Banner says
an encrypted error log	yes	enter your key to review the error log
ENCRYPTED-RESULT, no error log	yes	there is no error log, enter your key to review what the run did produce
ENCRYPTED-RESULT plus a plaintext or legacy error log	yes	a log exists but this screen cannot display it, enter your key to review what the run did produce
a plaintext or legacy error log only	no	a log exists, but this screen cannot display it
nothing, or only a security scan log	no	there is no error log for this run

The screen asks the decryptable question first. An ordinary packaging failure stores both halves of one log: the encrypted log for the key holder, and a plaintext twin. The screen reports that log as readable, not as a log that it cannot display.

The security scan log does not count towards the question. The code scanner writes it at submission. It already appears on the code review step. It says nothing about a run. Two problems came from counting it. An errored job that held only that log was told to review error logs that it did not have. The screen also offered "share outputs" for a run whose only artifact was a submission-time scan log.

Two unit tests check the rule across every combination of artifact types. The banner mentions a key exactly when the key form renders, and it never denies a log that the job carries.

Let the reviewer close the round

An errored job can hold nothing to decrypt. OutputsReviewPanel then skips the key step and shows the decision directly. The panel disables Share outputs and feedback and gives the reason. Share feedback only is the one option that remains. The server already permits this, because the "shares every artifact" check runs only on the sharing branch. The form still requires a selection, and it has no default. Closure of the round stays a deliberate act.

Two safeguards need a close review:

• The bypass reads the job's own files. It never reads an empty result from the artifact fetch. That fetch also returns empty when the caller has no registered public key, and when the fetch fails. If we treat those results as "nothing to decrypt", a reviewer with no key can skip decryption and decide on outputs that they never saw. OTTER-675 closed that hole. This case has a test.
• Each screen opts in to the bypass. Only the errored screen sets it. For a completed run, missing artifacts mean that delivery went wrong. They do not mean that there is nothing to review. The outputs-available screen therefore keeps its key gate. A test pins this behavior, because both screens share the panel.

Every ENCRYPTED-RESULT still needs a key. No reviewer can decide on outputs that they did not see.

Keep the decided screen honest

ReviewerOutputsDecided shows the post-decision "View outputs again" key form only when the job holds an encrypted artifact. A reviewer can now close out a run that produced nothing. Without this gate, a return to that page asks for a key against files that do not exist, and no key can open them.

The gate is the same rule as the errored screen, on the same predicate. The form promises the reviewer their outputs. A submission-time security scan log is not an output of the run. A decided job whose only encrypted artifact is that log therefore has no re-decrypt form. The log stays available on the code review step, which is where a submission-time scan belongs.

Deploy order

Either repository can merge first. Nothing breaks.

Order	Result
This PR alone, iac never merges	The screen shows the stage sentence and the no-log notice. Nothing breaks.
iac merges first	This app does not know the field yet, so Zod strips it. The job is still marked as errored.
This PR merges first	The field is optional and absent, so the fallback runs.
Both merged	The reviewer reads the specific sentence.

Two details hold this up. Please do not tidy them away:

• failureReason is an optional z.string(), not a z.enum. An enum rejects a code that this app does not know yet. The route then returns a 400, and the job is never marked as errored.
• An unknown value falls back to the stage sentence. It does not throw.

The buildspec also has a fallback path that always posts a bare payload. This app must therefore accept a webhook with no reason forever.

Delivery order does not matter, in either direction. A classified failure arrives twice. The build script posts from its own catch handler. The buildspec fallback then posts a bare payload, because post_build runs after the build phase exits non-zero. The reason survives in each order:

• classified first: the status dedup drops the bare duplicate, and the classified row stands.
• bare first: the dedup drops the classified duplicate. The route then records the code against the row that is already there.
• a second JOB-ERRORED row from any producer: the reviewer-scoped query selects the newest row that holds a known code, not the newest row. An enclave row or a /api/job/[jobId] row therefore cannot mask a reason when it sorts last.

The route keeps a classified code that a row already holds. It replaces unclassified text on that row, because the app never displays such text, and the text otherwise hides the one value that the screen can explain. Each of these cases has a test.

About the acceptance criteria

The card asks us to show the error log for a study that ran against a failed image or a deleted image. Both cases fail during packaging. As shown above, no error log exists on that path. With the companion iac PR, the reviewer instead reads a checked explanation of that exact failure. That explanation is the information that the AC asks for.

Product must confirm two points:

1. Is a checked reason acceptable in place of a literal error log, when the build produces no log?
2. AC1 says "an image where security scans have failed". That wording covers two different cases. The first case is an image that we cannot pull. That case is the reported one, and this PR covers it. The second case is an image that pulls correctly but fails a scan on findings. In that case the build succeeds, the code runs, and there is no error to show. If the second case was the intent, the card belongs to OTTER-525.

The merge with main

This branch now includes OTTER-696 (PR #972). That card moved four pieces of code into shared modules. This branch uses each shared module instead of its own copy:

Shared code from OTTER-696	Where this branch uses it
useDecryptPhase	OutputsReviewPanel and DecryptAndViewOutputs
statusAlertTitle	the title of the errored screen's banner
FeedbackAndNotesSection with a loadError prop	ReviewerOutputsDecided
PreviousStepLink	ReviewerOutputsDecided

OutputsReviewPanel keeps the gate that this card adds. The hook gives the decryption flip. The panel then applies requiresKey above the hook, because a run with no artifact has no key step.

The two cards do not overlap. They divide an errored run by what the reviewer can share:

• The job holds an encrypted artifact. The reviewer can share the outputs. The researcher then reads the new outputs-errored-shared screen from OTTER-696.
• The job holds nothing. This card disables Share outputs and feedback. Feedback only is the one possible decision, and the researcher goes to study-results.

This branch also removes one duplicate test fixture. Both cards wrote the same artifact fixture at the same time. shared-outputs-panel.test.tsx now calls seedEncryptedArtifact from tests/artifact.helpers.ts. That fixture makes the same zip and returns the same shape, so the suite asserts the same behavior.

Testing

pnpm run checks passes on this branch after the merge with main. pnpm run test passes in the iac repo, 13 tests.

The full unit suite passes locally: 292 files and 3419 tests.

One test needs a note for anyone who runs the suite on a reused local database. acknowledgements-table.test.tsx lists every user in the database, 25 to a page, sorted by name. Two of its cases look for their own user on the first page. The name comes from a generator, so the case passes only while the database holds few users. A local unit-test database that many runs have written to holds hundreds, and the case then fails. It passes on a fresh database. The test does not touch this branch, and it behaves the same way on main.

New coverage in this repo:

• stage derivation from the status history, and a job that errored before packaging began
• the banner sentence for each artifact shape
• the banner-and-gate rule and the never-deny-a-log rule, across every artifact combination
• the reported scan-log-only job: the copy, the missing key form, and the disabled share option together
• the plaintext-only log case, the same log beside a result, and a legacy plaintext result
• an errored run with results that still need a key
• the no-key decision path, and the two safeguards above
• the decided screen with a result, with a scan log only, and with nothing to decrypt. The scan-log case now expects no key form, for the same reason as the errored screen.
• a known failure code that renders our wording
• an unknown code and raw service text, both dropped instead of shown
• the webhook: it stores a known code, drops an unknown code, drops raw text, accepts no code, records a code that arrives after the bare fallback, keeps a code that it already holds, and records a code against a row that holds unclassified text
• the reason survives a later bare JOB-ERRORED row

We agreed not to run E2E.

Not in this change

• More failure classes. The build classifies only BASE_IMAGE_UNAVAILABLE. Every other failure keeps the general stage sentence, which is the safe default.
• The AWS stop reason from the enclave. One repository upstream replaces it with a fixed string. The fix has the same shape: classify at the source, and show our own words.
• A durable record of the image that a job used. StudyJob has no code environment reference. A foreign key cannot survive the case that this card names, because deletion of a code environment also deletes its scan history. A snapshot on the job is the right direction. It needs a migration, so it needs its own card.
• Display of a plaintext-only PACKAGING-ERROR-LOG, or of a legacy plaintext result, on this screen. Both are better than a sentence that says we cannot offer them. Each needs a new fetch path and new UI, and the reviewer flow has never supported them.
• A stop for the run at submission. OTTER-525 already specifies this. Its approach is better than a gate at approval time: an image must not become the default until its scan passes.
• A timeout for a job whose status webhook never arrives. That job hangs on "code is running" today. The symptom matches the dead end that this PR fixes, but the cause is different. It is worth its own card.
• The researcher side of an errored run whose outputs the reviewer withholds. OTTER-696 is merged, and this branch includes it. OTTER-697 is still open. A feedback-only decision releases the researcher from "code is running", which is the purpose of this escape. isFeedbackOnlyOutcome excludes an errored run. A run that this card closes with feedback only therefore goes to study-results. The researcher reads the FILES-REJECTED message about withheld results and PII, and the screen does not show the reviewer's feedback.
• Producer provenance for a classified code. PUT /api/job/[jobId] lets the org that owns the study write any message. A future producer that shares the enum can therefore have its value read as a containerizer classification. The sentences are advisory today, so nothing acts on the value. JOB_FAILURE_REASONS now carries that limit as a note, so the next person to add a code reads it first. A dedicated column or a source marker fixes the cause. That needs a migration, so it belongs on its own card.

Notes for review

• The iac PR deploys to QA when it merges. Review it as a pre-deployment gate. It edits a CodeBuild script that ships through the existing bucket deployment. It replaces no resources, and it destroys nothing.
• Two states ship without a Figma frame. This PR adds no new layout. It reuses the existing alert and decision components, with different copy and one disabled option.
• The copy is my wording. A product pass helps here. The copy sits in one module and one component, so a change is cheap.
• QA notices one behavior change. An errored job whose only artifact is a security scan log no longer asks for a security key. The outputs step can also no longer share that scan log. The log stays available on the code review step, which is the correct place for a submission-time scan.
• The recovery sentence names who can act. Any member of an enclave org can review, but the Code Environments page sits behind the org admin layout. The sentence therefore says that an organization administrator can check the image URL. It does not tell the reader to do it.

Overview

The reviewer's errored outputs screen now tells the truth about a failed run, explains the failure, and lets the reviewer finish:

• it names the stage that failed, from the status history. It still says something useful when the build produces no log, and it names no stage when the history cannot support one.
• when the build classifies the failure, a checked sentence replaces the general one. The screen shows only known codes, so no AWS detail or deploy detail reaches a screen that another organization reads.
• one rule decides what the banner promises and whether the key form renders. The two cannot contradict each other.
• a security scan log no longer counts as an explanation of a failed run
• a reviewer can close a run that produced nothing with feedback only. That decision also releases the researcher from "code is running".
• every encrypted result still needs a key, and each screen opts in to the bypass
• the decided screen offers the re-decrypt form only when a key can open something

[nathanstitt]

looks good, thanks Marvin

[nathanstitt]

Worth calling out explicitly in the code rather than only in the PR description: /api/job/[jobId] accepts an arbitrary message from any org-authenticated producer and writes it straight into this same column. So any enclave that knows the string can PUT { status: 'JOB-ERRORED', message: 'BASE_IMAGE_UNAVAILABLE' } and have latestRecordedJobFailureReason pick it up, and the reviewer reads a containerizer classification that the containerizer never sent.

The blast radius is small today (one code, and the sentence it produces is advisory), so I don't think this blocks. But the "not in this change" note about producer provenance is the kind of thing that gets lost once the PR is merged — could we drop a short TODO(OTTER-xxx) here or on latestRecordedJobFailureReason so the next person adding a code to JOB_FAILURE_REASONS sees that the column is not trusted-by-source?

[therealmarv]

Agreed, and the note is now in the code: job-error-details.ts L93.

It sits on the JOB_FAILURE_REASONS doc block, so the next person to add a code reads it before they add one.

I checked the route before I wrote the note, so the wording is narrower than the PR body was. PUT /api/job/[jobId] validates message as a free-form optional string. The handler then requires that the requesting org owns the study. So the writer is the study's own org, not any org-authenticated caller. The result is the one you describe: that org can send a code from this list, and the reviewer reads it as a containerizer classification.

The note states the rule for the next code, rather than only the current risk: the list is trusted by value, not by source. Before we add a code whose sentence a reviewer would act on, the classification needs its own column or a source marker.

The marker says TODO(OTTER-524). I have no write access to the board from here, so I did not invent a key. Tell me the number if you want a dedicated follow-up card, and I will re-point it.

[nathanstitt]

canShareOutputs={requiresKey} reads oddly — the two names answer different questions, and it took me a couple of passes to convince myself it's correct. It is: UnlockedPhase bails when decryptedFiles === null, so by the time canShareOutputs is consumed, requiresKey === true really does imply "decrypted, and there are files". But that's an invariant enforced two components away from where the prop is read.

Could we make the equivalence explicit rather than incidental? Something like a named local at line 80-88 —

// Same value as requiresKey: with no key step there is nothing decrypted, so nothing to share.
const canShareOutputs = requiresKey

— costs nothing and means a future edit to UnlockedPhase's null guard doesn't quietly turn a "has a key step" boolean into a sharing permission.

[therealmarv]

Done, close to your wording: outputs-review-panel.tsx L89-L93.

The named local carries the equivalence and the reason for it, so the prop site now reads canShareOutputs={canShareOutputs}. Your point about the distance is the part I wanted to fix. The invariant lived two components away, and nothing at the prop site said which question the value answered.

[nathanstitt]

Small robustness thing: JOB_FAILURE_REASONS is a single-element tuple today, and if it ever goes to zero (someone retires BASE_IMAGE_UNAVAILABLE before adding a replacement) Kysely renders where message in (), which Postgres rejects as a syntax error — so the reviewer's errored screen 500s rather than degrading to the stage sentence.

Cheap to make impossible: if (!JOB_FAILURE_REASONS.length) return null at the top, or keep the filter in TS by selecting the newest classified row's message and running it through isKnownFailureReason. Either way the failure mode becomes "no reason shown" instead of "page down".

[therealmarv]

Good catch. Fixed with the guard: queries.ts L158-L169.

The query returns null before it builds the statement, so the failure mode becomes "no reason shown" and the screen falls back to the stage sentence.

I kept the filter in SQL rather than moving it to TypeScript. The SQL filter is what makes the answer independent of which JOB-ERRORED row sorts last, and it is also why raw service text never leaves the database. Selecting the newest row and classifying it in TypeScript would give up both properties.

The array is typed as string[] on purpose. JOB_FAILURE_REASONS is a const tuple, so a length check against the tuple type reads as always false. Widening it keeps the guard honest to a reader and to the compiler.

[nathanstitt]

The wider predicate here means a decided job whose only encrypted artifact is ENCRYPTED-SECURITY-SCAN-LOG keeps its "View outputs again" form — and the test on line 236 of the spec pins that as intended. But that's the exact shape of the reported bug, just on the next screen: the reviewer enters a key, and what comes back is a submission-time scan log rendered under "outputs", which is the conflation the errored screen now refuses to make.

The comment argues this screen asks a narrower question ("does a key open anything"), which is fair, but the answer it gives the user is still framed as this run's outputs. Is a scan log actually something we want offered here, given the same log is already on the code review step? If it is, no change needed — but it'd be worth saying why it's useful post-decision rather than only why the predicate differs.

[therealmarv]

You are right, and I changed the behavior rather than defend it.

The screen now asks the same question as the errored screen: reviewer-outputs-decided.tsx L47-L53. The test now expects no key form for a scan-log-only job: spec L241-L250.

I could not answer your question with a use for it. The form says "The outputs are encrypted. Enter your security key to view them again." A reviewer who enters a key then receives a submission-time scan log under that promise. That is the same conflation, one screen later, and the log is already on the code review step. So there was nothing to explain, only something to remove.

What the narrow predicate costs is small. jobHasDecryptableRunOutcome still covers every ENCRYPTED-RESULT, the encrypted code run log, and the encrypted packaging error log. Only a scan-log-only job loses the form, and that job has nothing else for a key to open.

Two follow-on edits keep the story in one piece:

• the isVisible doc on DecryptAndViewOutputs now states the narrower question, since the prop name alone does not carry it
• docs/study-screens-logic.md said the gate was deliberately wider. It now says both screens use one predicate.

jobHasEncryptedArtifacts stays where it is. isLegacyResultJob still needs the wide question.

[nathanstitt]

APPROVED-SECURITY-SCAN-LOG is (correctly) left out of this list, matching the exclusion in ENCRYPTED_ERROR_LOG_TYPES — but nothing in the file says so, and the comment above only explains the PACKAGING-ERROR-LOG and legacy-APPROVED-* inclusions. Since the whole point of the module is that "any log" and "a log about a failed run" are different questions, the absence of the scan log from both sets is the load-bearing bit and the easiest thing for a future edit to undo. One line noting the deliberate omission would lock it in.

(The combinatorial test in job-error-details.test.ts covers SECURITY-SCAN-LOG but not APPROVED-SECURITY-SCAN-LOG, so adding it to CANDIDATES would pin it there too.)

[therealmarv]

Both parts done, and you named the right risk. The absence carries the meaning, and an absence is what a later edit removes without noticing.

• the comment states the omission and why: file-type-helpers.ts L75-L82. It says the scan log is not a log about a failed run in either form, and it tells the reader not to complete the APPROVED-* trio.
• APPROVED-SECURITY-SCAN-LOG now joins CANDIDATES: job-error-details.test.ts L185-L187.

The sweep now runs over eight file types, so both rules hold across 256 combinations each. Adding the type to either error-log set fails the first rule at once: the banner would offer a key that the gate does not render.

[github-actions]

Total coverage

Lines	Branches	Functions	Statements
96.42% (+0.06%)	77.23% (+0.29%)	92.82% (+0.10%)	95.71% (+0.07%)

Detailed report

9 files with a coverage regression

	File	Lines	Branches	Functions	Statements
🚫	src/app/[orgSlug]/study/[studyId]/_screens/reviewer-outputs-available-screen.test.tsx	100.00%	72.72%	97.05% (-0.17%)	100.00%
🚫	src/app/[orgSlug]/study/[studyId]/_screens/reviewer-outputs-errored-screen.test.tsx	100.00%	57.14% (-2.86%)	98.11% (+0.97%)	100.00%
🚫	src/app/[orgSlug]/study/[studyId]/_screens/reviewer-outputs-errored-screen.tsx	95.83% (+0.83%)	60.00% (-2.50%)	100.00%	95.83% (+0.83%)
🚫	src/app/[orgSlug]/study/[studyId]/review/reviewer-outputs-decided.tsx	100.00%	75.00% (-5.00%)	100.00%	100.00%
✅	src/app/api/services/containerizer/route.ts	100.00%	95.00% (+3.34%)	100.00%	100.00%
✅	src/components/editable-text/collaborative-editor.tsx	92.12% (+3.94%)	78.33% (+10.00%)	79.31% (+6.90%)	90.00% (+5.72%)
🚫	src/components/study/decrypt-and-view-outputs.test.tsx	100.00%	100.00% (+50.00%)	81.81% (-2.80%)	100.00%
🚫	src/components/study/outputs-review-panel.tsx	100.00%	94.73% (-5.27%)	100.00%	100.00%
🚫	src/components/study/shared-outputs-panel.test.tsx	100.00%	50.00%	92.59% (-0.51%)	99.29% (-0.06%)
✅	src/lib/file-type-helpers.ts	94.44% (+1.59%)	100.00%	94.73% (+2.43%)	94.44% (+1.59%)
✅	src/lib/job-error-details.test.ts	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
✅	src/lib/job-error-details.ts	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)	100.00% (+100.00%)
🚫	src/server/actions/editor.actions.ts	100.00%	66.66% (-16.67%)	100.00%	100.00%
🚫	src/server/db/queries.ts	92.36% (+0.36%)	68.29% (+0.73%)	92.72% (+0.13%)	90.44% (-0.25%)