Make an agent-driven repository run leave the same record as a GUI one - #23
Merged
Merged
Conversation
The architecture this app is built to is that a person uses the GUI, an agent uses MCP, and both drive the same functions. For repository runs they did not. prepare_run returns an explicit dict and repository_run_manifest was not in it. /api/run copied the key back onto the result by hand afterwards; /api/agent/run did not. _run_job gates the whole repository block on `if manifest_text:` -- the mzTab validation record, the retained-artifact inventory and the retention verdict all live inside it. So an agent-driven repository run wrote none of the three, and its raw data could never be released: cleanup_download_lease refuses unless cleanup_allowed is true and retained_artifacts exist, and both are written only inside that block. The same unit run from the GUI wrote all three. Nothing failed and nothing was logged; the record was simply thinner on the path every reanalysis agent uses. Fixed by carrying the two keys through prepare_run itself rather than copying them in a second place, so a caller cannot forget: there is no second place to remember. The GUI's manual copy is removed as redundant. The state already carries them on both paths -- msdial_prepare_repository_ reanalysis puts them in answers["workflow_overrides"] and build_guided_plan folds those into the workflow, which is also why the execution gate did hold on the agent path. It was only this last hop, from workflow state to preparation, that dropped them. A local laboratory analysis still prepares an empty manifest and "keep", which is what tells the gate there is nothing to gate and keeps a retention policy off data nobody downloaded. Tested, because defaulting that the other way would be worse than the bug. 274 tests pass. Blanking the carried key fails the new parity test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed MS-DIAL run recorded its status and its diagnosed error in the in-memory JOBS registry and nowhere else. That registry is persisted truncated to the hundred most recently updated jobs, and at the scale this is for -- each accession consuming a download job, a tuning job and one or more run jobs -- later work evicts the failure. The unit's own workspace then looks exactly like a unit nobody ever tried, which is the difference between "this one cannot be analysed" and "this one is still to do", and it is the question a hundreds-scale campaign asks on every iteration. The project contract requires "a failure record when unsuccessful" for every attempted unit. It existed only as prose telling an agent to write one: something judged that is never connected to what actually ran, which is the defect shape this programme is built against. Both failure paths now write it -- a non-zero exit and an exception -- into the analysis unit's run-manifest.json, with the diagnosed reason, the exit code, a timestamp and the last forty log lines. A local laboratory analysis carries no manifest and gets none of this; there is no unit to record against. CLEANUP STAYS FORBIDDEN, explicitly rather than by omission. The raw data is what a retry needs and a failure is exactly when the disk looks reclaimable. The campaign's retention policy is now "delete after a successful run"; this is the clause that keeps the word "successful" in it. run_failed is also not among CLEANUP_READY_STATUSES, so the two guards agree. Failures accumulate rather than overwrite: a unit that failed three times is a different fact from one that failed once, and the difference is what says whether to keep trying. Recording never raises. Failing while recording a failure would lose both, and the caller is already on its error path. 274 tests pass, 5 new. Letting cleanup_allowed stand fails the guard test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cleanup_download_lease's deletion preview reports
`manifest.get("raw_retention_policy")`. Nothing had ever written that key.
So the one field that says whether a unit's raw data was ever meant to be
deleted read None in the preview a person is shown before confirming the
only irreversible operation in this pipeline.
The policy was held in the in-memory JOBS registry alone, which is persisted
truncated to the hundred most recently updated jobs. At campaign scale --
each accession consuming a download job, a tuning job and one or more run
jobs -- later work evicts the policy while the data it governs is still on
disk. A field that is read and never written, and a decision that outlives
its own record: the two shapes this programme keeps finding, in one place.
Written into the unit's run-manifest.json at download time, which is the
unit's own durable record and outlives every registry. The CLI gains
--raw-retention-policy with the same two legal values the HTTP endpoint
already validates, so the script path records it too.
WHAT THIS DOES NOT DO is authorise anything. The policy is a wish; the
guards are status, cleanup_allowed, a non-empty retained-artifact inventory
whose files are verified to still exist, and the raw directory being the one
inside the unit's own workspace. A unit that asked for deletion and has not
validated is still refused, and that is tested. The default stays "keep", so
an omission never authorises a deletion.
A manifest written before this change carries no policy and reports None,
which is the honest answer -- the intent genuinely was not recorded -- and
does not become a blocker, because the policy was never one of the guards.
285 tests pass, 6 new. Dropping the write fails two of them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An adversarial audit of the deletion path found no way to delete without confirmation and nothing broken in the chain -- and one silence. The run path compared the policy against a bare literal with no strip and no casefolding, so "delete", "Delete" and "delete_after_validated_output " with a trailing space were all keep-equivalent. The run then logged "downloaded repository raw data were kept": true, and no help whatever to someone who believed they had asked for deletion. The unreadable request survived only in a job registry truncated to a hundred entries, so at full-repository scale it vanished without a word. The governed chain cannot produce this -- the download endpoint validates, and msdial_prepare_repository_reanalysis reads the policy back off that validated job rather than taking it as a parameter. The exposure is a caller that hand-writes answers["workflow_overrides"], which is accepted as free-form dict content with no schema. normalize_raw_retention_policy resolves an unreadable request towards KEEPING, because the only alternative is irreversible, and returns whether it was understood so the caller can be told. prepare_run puts that in its warnings, which reach a caller before the run rather than a log tail after it. The two legal values are now named once and the download endpoint validates against the same tuple, instead of an inline set that a third reader could disagree with -- which is how this arose. AND THE AUDIT CAUGHT MY OWN TEST. The parity test I added earlier today pinned "repository_raw_retention_policy": "delete" -- a string that means nothing anywhere in this codebase. It passed, and it had no opinion about the one value that is not keep-equivalent. Corrected to the real literal, which made it fail against the new normalisation until the value was right: the test now holds what it claimed to hold. Audit result: nothing unsafe, nothing broken. Deletion requires status, cleanup_allowed, a non-empty retained-artifact inventory whose files are verified to still exist on disk, the raw directory being the one inside the unit's own workspace, and confirmed=true from a caller -- never from the server. 290 tests pass, 5 new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Python float prints its decimal point even when it names a whole number, so a minimum peak height of 500 reached the method file as "500.0". MS-DIAL's Console parses that key with int.TryParse and, until today, reported the key as consumed whether or not the parse had succeeded -- so the value was discarded, MinimumAmplitude kept its built-in 1000, and the retained method file, the publication report and the Methods paragraph all recorded 500. Every threshold the contract's zero-threshold diagnostic has ever chosen was thrown away exactly this way, and no artifact in the workspace could contradict the number. Every method.txt under the reanalysis workspace shows it: 300.0, 500.0, 1000.0. The Console reader has been fixed to accept either spelling. This is the other half, and it is the half that takes effect first: the Console the pipeline resolves is built from a branch that takes changes from master later, so a method file written today is read by yesterday's parser. Only the rendering changes. A fractional value keeps every digit -- a tolerance of 0.01 is still 0.01 -- and a bool is still True or False, which is the case that would have broken quietly, because bool is an int in Python. One existing assertion pinned "2.0" for a whole-numbered RT tolerance. That key's C# arm is float.TryParse and reads either spelling, so the assertion was pinning the incidental formatting; it now asserts "2" and, beside it, that 0.5 still writes as 0.5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The project contract requires a zero-threshold diagnostic before every production repository run, and requires the method, representative sample, diagnostic count, threshold step and accepted threshold to be retained in provenance. All five were computed and none of them reached the workspace. The estimate went into the HTTP response and into the in-memory JOBS registry, which is persisted truncated to the hundred most recently updated jobs -- so at campaign scale, where each accession consumes a download job, a tuning job and one or more run jobs, the measurement was evicted while the run it justified was still on disk. What survived was the number alone, carried by hand from a tool response into the production answers. An audit reading the retained artifacts could see that a run used a threshold of 500 and could not see whether 500 had ever been measured on this unit, on a different unit, or at all. That is the shape this programme keeps finding, in the one place the contract names explicitly. record_peak_height_diagnostic appends to the unit's own manifest. Appended, because re-running with a different step or a different representative is ordinary, and which thresholds were considered is part of why the accepted one was accepted. The estimator's whole result is kept unedited beside the named fields, because selecting fields here is how a later change to the estimator silently stops being recorded. A laboratory analysis has no repository manifest; nothing is written and the response says so rather than leaving it to look like a write that succeeded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five fixes on the repository-reanalysis path, all found by auditing what an
agent-driven run records versus what a GUI-driven one records.
The architecture this app is built to is that a person uses the GUI, an agent
uses MCP, and both drive the same functions. For repository runs they did not,
and the difference was invisible: nothing failed, the record was simply thinner
on the path every reanalysis agent uses.
What was wrong
The agent path dropped the unit manifest.
prepare_runreturns an explicitdict and
repository_run_manifestwas not in it./api/runcopied the key backby hand afterwards;
/api/agent/rundid not._run_jobgates the wholerepository block on
if manifest_text:— the mzTab validation record, theretained-artifact inventory and the retention verdict all live inside it. So an
agent-driven run wrote none of the three and its raw data could never be
released.
A failed run left nothing where the unit lives. Status and the diagnosed
error went to the in-memory
JOBSregistry, which is persisted truncated to thehundred most recently updated jobs. At campaign scale later work evicts the
failure and the unit's workspace looks exactly like a unit nobody tried. The
contract requires a failure record for every attempted unit; it existed only as
prose telling an agent to write one.
The retention policy was not durable.
cleanup_download_lease's deletionpreview reports
manifest.get("raw_retention_policy")and nothing had everwritten that key, so the field that says whether a unit's raw data was ever meant
to be deleted read
Nonein the preview shown before the only irreversibleoperation in this pipeline.
An unreadable policy was silently keep-equivalent. The run path compared
against a bare literal with no strip and no casefolding, then logged that the
data were kept — true, and no help to someone who believed they had asked for
deletion.
How they are fixed
prepare_runcarries the two keys through itself, so there is no second place toremember and the GUI's manual copy is gone. Failures are written into the unit's
run-manifest.json, accumulating rather than overwriting, withcleanup_allowedset false explicitly — the raw data is what a retry needs. The policy is written
into the manifest at download time, where it outlives every registry, and the CLI
gains
--raw-retention-policy. An unreadable policy resolves towards keepingand reports that it was not understood, through
prepare_run's warnings, whichreach a caller before the run rather than a log tail after it. The two legal
values are now named once.
What this does not change
Nothing here authorises a deletion. An adversarial audit of the deletion path
(32 agents, four angles) found no way to make it fire without confirmation and
nothing broken in the chain. Deletion still requires a validated status,
cleanup_allowed, a non-empty retained-artifact inventory whose files areverified to still exist on disk, the raw directory being the one inside the
unit's own workspace, and
confirmed=truefrom a caller — never from the server.The default stays
keep.That audit also caught a regression test added in this branch pinning
"delete"— a string that means nothing anywhere in the codebase. Corrected tothe real literal, which made it fail until the value was right.
290 tests pass, 16 new. Each fix has a mutation check recorded in its commit.