Repository navigation
File inbound .processed archives get a default-on 30-day age bound (BACKLOG #1188) - #2581
wshallwshall wants to merge 13 commits into
Conversation
Owner ruling A2 of 2026-10-08 for BACKLOG #1188 (research an honest pass for ASVS 14.2.7). Adds [retention].processed_archive_days as a fourth auto-bound window at the body window (30 days), keyed on archive time: the File source stamps mtime before each .processed move. The retention pass sweeps each bound File inbound's processed_dir one level deep, regular files only, never through a link, and refuses a directory that is or holds any File inbound's watch or .error dir. .error stays unswept (#3335). Pinning tests move from three windows to four; PHI.md and CONFIGURATION.md updated. WIP: code-review skill and mypy legs not yet run on this commit. Proposed PR title: Retention: default-on .processed age bound (owner ruling A2, BACKLOG #1188) Proposed banner for #1188: ENGINE BUILT 2026-10-27: owner ruling A2's .processed bound; a vault re-score of the cell is owed
…passes The fixture carried a type-ignore that strict mypy on the tests leg reports as unused; give it an AsyncIterator[MessageStore] return instead. BACKLOG #1188
… handle Repairs from code-review round 1 on the .processed age bound: - The overlap guard now names the watch and .error directories of every registry inbound, bound or not, so a stopped, failed, parked or other-shard inbound stays protected; an unresolvable one skips the sweep. - Directory keys are case-folded on every platform, resolved once per pass off the loop, and the opened archive is also checked by identity. - Only after_read="move" sources are swept. - The POSIX stamp uses UTIME_NOW (write access, not ownership). - Windows deletes through a handle opened as itself, read-only included; POSIX re-checks identity and mtime before the unlink. - A listing fault keeps the count so far; failures are logged as a count; the refusal warns every pass; a stop or cancel ends the sweep between files; a credential fault no longer aborts the pass. BACKLOG #1188
Repairs from code-review round 2 on the .processed age bound: - ArchiveGuard carries resolved keys and the (dev, ino) of every guarded directory and its parents; the opened archive is refused by identity, so a UNC/local alias, bind mount or x/.. of another inbound's watch directory is caught. An unresolvable path falls back to its absolute form instead of switching the whole sweep off. - POSIX keys on the later of mtime and ctime, so a stamp the engine may not make on a partner-owned drop changes nothing; its log is debug. - Only archiving (move) sources are swept or claim a shared directory; the dedup key is not case-folded. - The guard fails closed only for connectors that declare directories. - Setup faults are contained; FileSource raises credential faults as OSError, so pipeline imports no transport exception. - The read-only delete flag retries without it on builds that lack it. BACKLOG #1188 Proposed PR title: File inbound .processed archives get a default-on 30-day age bound (BACKLOG #1188) Proposed ledger banner: ENGINE BUILT 2026-10-27: owner ruling A2's .processed age bound; a vault re-score of the cell is owed
… guards The WIP commit reworded the PL-1 spill-dir sentence and named the .processed sweep row by a method that is not a Store method, which red three tests in test_phi_at_rest_inventory. Restore the phrase the test pins, name the row by its window, list that window as a non-store operation, and state the POSIX ctime keying in the row. BACKLOG #1188 Proposed PR title: File inbound .processed archives get a default-on 30-day age bound (BACKLOG #1188) Proposed ledger banner: ENGINE BUILT 2026-10-27: owner ruling A2's .processed age bound; a vault re-score of the cell is owed
|
QA (BACKLOG #1188), rounds 1-2: code-review skill at level xhigh. Round 1 nonce round-a418729a, round 2 nonce round-7dbeb8c6; korus review_tag.py exit 0 both; tag verbatim both rounds: "xhigh effort -> 10 inline angles -> dedup (no verify) -> sweep -> <=15 findings". Inline review with no verifier pass, per the tag. Round-2 repairs (261ac03) and the PHI.md fix (2539a23) are not yet covered; a covering review is running and its QA line will follow. qa label withheld until then. |
| for ic in self.registry.inbound.values(): | ||
| if not declares_guarded_dirs(ic.spec.type): | ||
| continue # a fault in an inbound with no directories cannot hide one | ||
| try: | ||
| settings = resolve_env_settings( | ||
| ic.spec.settings, self._env_values, connection=inbound_record_name(ic.name) | ||
| ) | ||
| dirs.extend( | ||
| guarded_source_dirs(Source(type=ic.spec.type, name=ic.name, settings=settings)) |
There was a problem hiding this comment.
Other engine shards lose protection
archive_guard_dirs() only sees this engine shard's inbounds because filter_registry_for_shard() removes the others before the runner receives the registry. If inbound A's processed_subdir equals inbound B's watch or .error directory on another engine shard, B is missing from both guard lists. A's new sweep would then delete B's aged files, including drops B has not read.
Preserve the full configuration's guarded directories before filtering, and test this case across two engine shards.
Prompt To Fix With AI
This is a comment left during a code review.
Path: messagefoundry/pipeline/wiring_runner.py
Line: 3062-3070
Comment:
**Other engine shards lose protection**
`archive_guard_dirs()` only sees this engine shard's inbounds because `filter_registry_for_shard()` removes the others before the runner receives the registry. If inbound A's `processed_subdir` equals inbound B's watch or `.error` directory on another engine shard, B is missing from both guard lists. A's new sweep would then delete B's aged files, including drops B has not read.
Preserve the full configuration's guarded directories before filtering, and test this case across two engine shards.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Round 2 replaced _dest_holds, which swept nothing when the opened archive directory could not be stat'ed, with _opened_id, which returned None and so matched no guard. A directory reporting inode 0 also never matched, because build_archive_guard drops inode-0 identities. Both cases now skip the connection for the pass with a warning, failing closed as before. Also: the wrapped credential/runtime fault no longer blames an alternate credential the connection may not have; the retention docstring no longer says an unresolvable directory skips the whole sweep; the PHI inventory exemption for processed_archive_days is bound to RetentionRunner and FileSource methods, not only to the setting name in a log string; and a test pins the real _archived_at, which the autouse fixture replaces. BACKLOG #1188 (cover review of 261ac03 and 2539a23).
…rocessed CONFIGURATION.md and PHI.md called ctime the archive time. Any later change to a copy's metadata (chmod, chown, an ACL change) also moves ctime and restarts that copy's window on POSIX; say so. CONFIGURATION.md now also states the Windows failed-stamp case PHI.md already admitted. BACKLOG #1188 (cover review of 261ac03 and 2539a23).
|
QA (BACKLOG #1188), covering review of 261ac03 and 2539a23: code-review skill at level xhigh, nonce round-77530dba, korus review_tag.py exit 0, tag verbatim: "xhigh effort -> 10 inline angles -> dedup (no verify) -> sweep -> <=15 findings". 15 findings; one real regression fixed (identity check fail-closed again) plus doc/test fixes in e758317 and eda938a. Those fix commits reviewed in round-73a302db, review_tag.py exit 0, same tag. Every commit on the branch now has review coverage. Known defects shipped as critic notes (round cap): the ino-0 refusal stops the sweep on a volume with no file IDs, or on an ACCESS_DENIED stat fallback, and the docs do not list that skip (Manager decision: keep fail-closed, the strictly safer option); an unstat-able guarded dir adds no identity; the inventory pin is name-only; PHI.md still says 'cannot be resolved' skips the whole sweep and the section 8 row lacks the Windows caveat. Declined findings and reasons are in the Builder reports; unfiled follow-ups: POSIX ctime can defer deletion under recurring metadata touches; outbound dirs unguarded; archive inside a recursive inbound tree. Linux pytest leg must be read (POSIX ctime test skipped on Windows). |
|
Lander (local_728989d9), 2026-10-28. HELD for author fixes (adversarial review: the strictly safer option). Main never deletes from .processed; this PR adds a default-on 30-day sweep, so each guard gap is a NEW irreversible-deletion risk:
|
…losed on stat Three gaps in the .processed age sweep's guard, each a deletion risk: - Under serve --shard the runner's registry holds only this shard's inbounds. The shard filter now pins every inbound's spec (all_inbound_specs) and archive_guard_dirs reads Registry.inbound_specs(), so a sibling shard's watch and .error directories are guarded. - Every File outbound's target directory is guarded, through a guarded_dirs declaration on FileDestination. - build_archive_guard raises ArchiveGuardUnresolved when a guarded directory exists but cannot be stat'ed, so the sweep deletes nothing. An absent directory stands in by its nearest existing ancestor; Windows ERROR_BAD_NETPATH, which maps to ENOENT, is not read as absent.
…able dirs PHI.md and CONFIGURATION.md now say the sweep skips an archive directory that overlaps any inbound's watch or .error directory on every engine shard, or any File outbound's target directory, and skips the whole sweep when a guarded directory exists but its identity cannot be read. Lander hold fixes 1-3 on PR 2581 (BACKLOG #1188).
…ve only dir keys Review round 1 on the guard fixes: - An unreadable guarded directory no longer stops every archive. The guard records it as unresolved; each archive reads it again under its own identity (an alternate credential may reach a share the service account cannot) and refuses only if it still cannot. A traverse-only ancestor above a readable directory is passed over. Each directory is stat'ed once per guard build. - archive_guard_dirs resolves only the settings a connector declares its directories from (guarded_setting_keys), so a not-deployed connection's unrelated env() secret is neither read nor able to switch the sweep off. The inbound and outbound loops are one loop. - PHI.md points at CONFIGURATION.md for the refusal list, which now says "at least". BACKLOG #1188.
… keys resolve all Review round 2 on the guard fixes: - A guarded directory whose stat reports inode 0 is unresolved: Windows answers a refused directory from its parent's listing with inode 0 rather than raising, which passed the fail-closed check silently. - An archive with no alternate credential refuses on an unresolved directory without re-stat'ing it; only a credentialed one re-reads. - A connector that declares guarded_dirs but not guarded_setting_keys has every setting resolved rather than a partial view. - Dead ArchiveGuard.__or__ removed; the settings comment and the inode-0 log line no longer understate the guard; test notes corrected. BACKLOG #1188.
A directory first reached as an ancestor whose stat reports inode 0 was added to the walk's memo with no identity, so a later guarded directory at that path counted as found and was never unresolved, bypassing the inode-0 fail-closed rule. Only a directory with an identity is memoised now. BACKLOG #1188.
|
Manager 784448: Lander hold addressed. New head e51835e, a fast-forward from 454b8d5, checked with ls-remote. Fixes (each one only widens what the sweep refuses):
Tests: 8 new tests seen failing on 454b8d5 and passing after. The inode-0 ancestor test was seen failing on 90b1b6e. QA (covers the final head): code-review skill at level xhigh, nonce round-e6339e2c, on e51835e against origin/main. korus review_tag.py exit 0. Tag verbatim: "xhigh effort -> 10 inline angles -> dedup (no verify) -> sweep -> <=15 findings". 15 findings. Inline review, no verifier pass, per the tag. Earlier rounds: round-ea84af76, round-674132d1, round-a64cbb92 (that last one found the inode-0 regression fixed in e51835e). Open findings that change behaviour or weaken a control, none introduced by the repairs. For the Lander to judge and file:
|
Merge danger
Medium. This adds a default-on retention control that deletes files from File inbound
.processedarchive directories once they pass the age bound. It is a security and PHI control, so it gets its own PR. It shares files with several open engine PRs: at least 2564, 2565 and 2562 (docs/PHI.md,docs/CONFIGURATION.md,config/settings.py), and others touchingsettings.py,engine.pyandwiring_runner.py.git merge-treeagainstorigin/mainat open time was clean. A covering code review of the last two commits (261ac03, 2539a23) is running. Theqalabel goes on once it reports.Evidence
mypy is clean on all four legs at the head:
scripts/asvs: 6 files.ruff is clean. Targeted pytest runs:
-k "file or retention or wiring_runner or engine_start": 3281 passed. Its one failure (test_phi_at_rest_inventory) was fixed in 2539a23, and that file now passes;-k "phi or doc_drift or configuration or docs": 2140 passed, 93 skipped.The full suite was not run. Mutation checks: removing the stamp, removing the overlap guard, the read-only-delete test, and the identity-guard test each turned tests red.
BACKLOG #1188: research an honest pass for ASVS 14.2.7 -- the three classified PHI tiers no sweep reaches, under the 2026-07-30 no-auto-bound ruling
This implements owner ruling A2 of 2026-10-08: a default-on age bound on
.processed, added as a fourth auto-bound window at the body window and keyed on archive time..errordeletion is out of scope; it was never ruled. Batch 447. Source branchb447-1188-processed-age-bound, head2539a23f6a5da8596ca5b412f1b33eb403281980.The setting and the default:
[retention].processed_archive_days, added as a fourth window inPHI_RETENTION_WINDOWSwithauto_bound_days=30, the same as the body window.allow_keeping_phi_indefinitelyis set.MIN_PHI_RETENTION_WINDOWSgoes from 10 to 11.Archive-time keying:
FileSource._move(stamp=True)sets the pinned file's mtime to now just before the move into.processed.UTIME_NOWon the directory fd, without following links, and the sweep keys on the later of mtime and ctime. Windows setsFILE_BASIC_INFOthroughReOpenFile.The sweep:
FileSource.prune_processed_archiveruns off the event loop through_run_fs. It deletes regular files one level deep, never following a link, and deletes through a handle on Windows._open_destand checked by identity..errordirectory. The check compares file identity over every protected directory and its parents, and folds case.max_pass_seconds, cancels between files, and logs counts only, never file names.processed_archive_daysandprocessed_archives_deleted.Docs:
docs/PHI.md(section 2 row split, a section 8 row, the purge table) anddocs/CONFIGURATION.md(retention section only).Legs to read: the linux pytest leg, because
test_posix_ages_a_copy_from_its_ctimeand the POSIXdir_fdpaths only ran their types on Windows. Also the mypy legs andwindows-service-smoke.Proposed ledger banner: "ENGINE BUILT 2026-10-27: owner ruling A2's .processed age bound; a vault re-score of the cell is owed".
Review findings declined, with reasons:
max_pass_secondsbounds it, and batching it would be a redesign._pin_confineddocuments.Unfiled follow-ups, for the Lander to file as rows: (c) and (d) above.
Fix the cross-engine-shard directory guard before merging; the new sweep could delete another inbound's unread files.
Fix with agent prompt
Summary
Adds a fourth default-on, 30-day retention window for File inbound
.processedarchives..processedarchives using an archive-time age limit.Acknowledged scope and trade-offs from
wshallwshall:Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD Config[Full inbound configuration] --> Filter[Filter for this engine shard] Filter --> Runner[RegistryRunner] Runner --> Archives[Bound processed archives] Runner --> Guard[Guard directories from filtered inbounds] Archives --> Sweep[Delete aged archive files] Guard --> Sweep Other[Other engine shards' watch and error directories] -. Missing from guard .-> GuardReviews (1) · Last reviewed commit: "docs: keep PHI.md's spill-dir sentence a..." · Reviewed by Greptile