review(#4213): every reader of a declaration fails closed, and the window is read off the purge

[rbuergi]

Stacks on #4213, which was already in the merge queue when its review landed and therefore
could not be pushed to ("Branches that are queued for merging cannot be updated"). Dequeuing a
green, queued PR to amend it is a shared action with a steward attached, so the fixes come as
their own change instead. Until #4213 merges this diff shows both commits.

Six Copilot findings on #4213 and #4214. All six are real; each has its own falsified arm.

1–2. 🚨 Fail-open on a malformed switch — three guards disarmed by one quote mark

Every reader of pause and recordAheadOfRegistry asks inForce is True. That is correct for a
JSON boolean and catastrophic for "true", which is not True. A record written with a typed
quote mark would read as no declaration at all to:

• the apply interlock — it would walk straight past a pause and enable the purge;
• the record overwrite guard — it would recapture the live 7-day task without OVERWRITE;
• the new coherence gate — it would pass an Enabled task sitting under an apparent pause.

…while reading, to a human reviewing the diff, exactly like an armed declaration.

The shape is now asserted on every pull request (check_in_force_is_boolean, over both blocks:
not-an-object, missing inForce, and a non-boolean are each RED), and both shell halves fail
closed on the same condition rather than trusting that to have run. Controlled against a scratch
record:

$ cat tasks.json   # pause.inForce is the STRING "true", and a task is Enabled
$ acr-retention-tasks.sh apply
pause.inForce is 'true', not a JSON boolean — refusing to decide whether the purge is paused
exit 1
$ acr-retention-tasks.sh record     # recordAheadOfRegistry.inForce likewise
::error::cannot read …/tasks.json, so it is unknown whether the record is
  deliberately ahead of the registry. Refusing to decide either way.
exit 1

3. 🚨 The window was read off the shell LINE, not the command that deletes

cmd: echo --ago 30d; acr purge --filter 'x:.*' --untagged satisfied the 30-day floor with the
purge carrying no --ago at all. Each acr purge is now isolated at the first shell separator and
checked on its own — and every invocation on the line, so a second one carrying --ago 7d no
longer rides in behind a compliant first.

4. --keep=10 and a bare --keep escaped a whitespace-only match

Both are quotas; a bare one is a malformed step rather than a compliant one. All three spellings
count now, and a bare --ago with no duration is unchecked — which is a failure, never a pass.

5. 🚨 The declared-drift exemption was keyed only by task name

Once purge-old-images was listed under recordAheadOfRegistry, every difference in its live
YAML printed as the deliberate record-ahead drift and left verify green — including a live
definition that had gained --include-locked (which deletes every manifest the lock protects),
swapped its filters, or stopped being an acr purge. The record-side checks read the recorded
file, so they would never have seen it.

The LIVE steps of a declared task are now held to all three invariants. The declaration covers the
window moving, never the task becoming something else.

6. (#4214) "two AZURE_*" — there are three

Corrected in #4222, with the denominator counted rather than eyeballed: the preflight carries nine
missing+= lines, five already provisioned for main-cd and four COMBO_* missing.

The negative controls

Restoring the three pre-review behaviours — the window read off the whole line, the whitespace-only
--keep, and the inForce boolean check removed — reds seven arms:

::error::ARM 31c: a `--ago` sitting on the LINE but not in the `acr purge` INVOCATION satisfied the window…
::error::ARM 31c: `--keep=10` passed. It is the same quota as `--keep 10`…
::error::ARM 31c: a BARE `--keep` passed — a malformed step is not a compliant one
::error::ARM 31c: a SECOND `acr purge` on the same line escaped the window check…
::error::ARM 31d: `pause.inForce` as the STRING "true" was accepted…
::error::ARM 31d: `recordAheadOfRegistry.inForce` as the STRING "true" was accepted…
::error::ARM 31d: the coherence gate passed an `Enabled` task sitting under a pause whose `inForce`
         is a string — the exact record the apply interlock would also walk past

Restored: --self-test exits 0. The shell halves were driven against a scratch record (above), no
Azure.

Verification

• lock-pinned-digests.py --self-test → 0 · --check-retention-record . → 0
• check-workflow-shell.py --root . → 0 (shellcheck covers .github/scripts/*.sh)
• dotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserror → 0 Warning(s) 0 Error(s)
• dotnet test … → 447 passed, 0 skipped

Refs #3438
Refs #3859

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect declaration handling, shell command parsing, and retention reporting.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates ACR retention controls and documentation to enforce a 30-day, no-quota purge policy with fail-closed interlocks.

Changes:

• Adds retention-window, pause, and coherence validation.
• Guards recording, verification, and re-enabling operations.
• Updates retention records, architecture docs, and release notes.

File summaries

File	Summary
src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-13-retention-keeps-continuous-artifacts-for-thirty-days.md	Adds the user-facing retention change note.
src/MeshWeaver.Documentation/Data/Architecture/ReleasedArtifactRetention.md	Updates retention status documentation.
src/MeshWeaver.Documentation/Data/Architecture/PinnedImageRetention.md	Clarifies recorded and live retention policy.
src/MeshWeaver.Documentation/Data/Architecture/ArtifactRetentionInterlock.md	Documents policy and interlock behavior.
.github/scripts/lock-pinned-digests.py	Validates retention policies, declarations, and self-tests.
.github/scripts/acr-retention-tasks.sh	Guards recording, verification, and re-enabling.
.github/acr-retention/tasks.json	Records pause and registry-drift state.
.github/acr-retention/purge-old-images.yaml	Updates the purge window to 30 days.

Review details

Suppressed comments (8)

.github/scripts/acr-retention-tasks.sh:294

• The apply interlock has the same null/absent ambiguity for pause: with "pause": null, p is None selects the no-pause path and an enabled task can reach the mutation prompt. This should fail closed for a present null, while only an absent pause block should print no.

p = d.get('pause')
if p is None:
    print('no'); sys.exit(0)

.github/scripts/acr-retention-tasks.sh:178

• The declared-drift path repeats the same text-matching flaw: grep -qv "acr purge" accepts a live cmd: echo "acr purge --filter ...", and the filter comparison can still match the recorded task. A live task can therefore become a no-op while verify reports the drift as declared and exits successfully. Validate the actual command tokens rather than the substring.

        if printf '%s\n' "$live_steps" | grep -qv "acr purge"; then
          echo "::error::the LIVE definition of '$task' has a \`cmd:\` step that is not an acr purge."
          drift=1

.github/scripts/acr-retention-tasks.sh:186

• The declared-drift exemption compares only single-quoted --filter '…' text. A live task can retain all recorded filters and add an unquoted or double-quoted --filter for another repository, leaving live_filters == recorded_filters; because declared drift is otherwise accepted, verify would then greenlight an unreviewed purge target. Compare parsed filter arguments (including every spelling), rather than extracting only this quote form.

        local live_filters recorded_filters
        live_filters=$(printf '%s\n' "$live_steps" | grep -o -- "--filter '[^']*'" | sort | uniq)
        recorded_filters=$(grep -E "^[[:space:]]*-[[:space:]]+cmd:" "$recorded" \
                             | grep -o -- "--filter '[^']*'" | sort | uniq)
        if [ "$live_filters" != "$recorded_filters" ]; then

.github/scripts/lock-pinned-digests.py:1757

• After recording a malformed pause, this function still calls .get on the raw value. A truthy non-dict such as "yes" makes check_pause_coherence raise AttributeError, so --check-retention-record crashes with a traceback instead of returning the collected validation error. Normalize non-dict pause values before reading their fields.

    pause = manifest.get("pause") or {}
    in_force = pause.get("inForce") is True

.github/scripts/lock-pinned-digests.py:1656

• Stopping the regex at ; only isolates the purge for its window check; it does not reject other commands on the same cmd: line. A recorded step such as acr purge ... --ago 30d --untagged; az acr repository delete ... passes the substring, window, and lock checks while adding an unrelated destructive operation. The record gate must validate the complete shell command and reject non-allowed commands.

# command that deletes. Each `acr purge` is isolated at the first shell separator and checked on its
# own, and EVERY invocation on the line must pass.
PURGE_INVOCATION_RE = re.compile(r"acr\s+purge\b(?P<args>[^;&|\n]*)")

.github/scripts/lock-pinned-digests.py:1660

• These matchers now accept --ago=30d, --keep=10, and a bare --keep, but retention_windows() still extracts only the spaced forms (--ago (\S+)/--keep (\S+)). An enabled record using an accepted equals spelling would therefore be reported as --ago ? or --keep none, contradicting the policy reader and misleading the protection report. Reuse the same parser for the report.

# 🚨 BOTH SPELLINGS, AND THE BARE FLAG. `--keep=10` is the same quota as `--keep 10`, and a bare
# `--keep` is a malformed step rather than a compliant one; a whitespace-only match passed all three.
KEEP_RE = re.compile(r"--keep(?:[=\s]+(?P<value>[^\s;&|]+))?")
AGO_RE = re.compile(r"--ago(?:[=\s]+(?P<value>[^\s;&|]+))?")

.github/scripts/lock-pinned-digests.py:2884

• The success summary still says the self-test has “25 arms”, but this change adds the ARM 31/31b/31c/31d retention checks described later in the same message. That output now understates the coverage it claims to report; update the count or derive it instead of leaving a stale verification summary.

          "and counted as protecting NOTHING, and the two existing pin extractors still agree. AXIS 3: the set an installation is RUNNING is locked though no file pins it, its migration twin with it, the TAG is locked beside the manifest, an installation that did not answer is INCOMPLETE and refuses the unlock arm, silence is never retirement, a stale roster entry and an unknown running set are RED, the digest extractor is controlled against a fixture rather than inferred from the fleet, a tag lock that did not take is counted as protecting NOTHING, a locked INDEX is expanded to the platform manifests acr-cli would otherwise collect out from under it, and the harness provably drives the same path as run(). THE RECORD: every recorded purge step is held to #3842's decided window — at least 30 days by age, no `--keep` build-count quota — with the exact `--ago 7d --keep 10` step this repository carried until 2026-09-13 driven as a literal control, a bare or unreadable `--ago` RED, and the decided window itself proven to PASS; and the pause declaration cannot contradict the statuses it is recorded beside, in either direction. The window is read off each `acr purge` INVOCATION rather than the shell line, every invocation on it, `--keep=N` and a bare `--keep` count as quotas, a bare `--ago` is unchecked rather than compliant, and a declaration whose `inForce` is the STRING \"true\" — which every `is True` reader silently treats as absent — is RED in both blocks.")

src/MeshWeaver.Documentation/Data/Architecture/ArtifactRetentionInterlock.md:418

• acr-retention-tasks.sh apply does not lift the pause in the current record: it writes the YAML and then applies status: Disabled for purge-old-images; the interlock only refuses when a paused record also has an Enabled task. Thus applying the decided window and lifting the pause are not the same act. Clarify that apply can update the disabled definition, while re-enabling still requires the reviewed pause/status change.

1. **The decided window has not been APPLIED to the registry.** The record states it; the live
   (disabled) task still carries `--ago 7d --keep 10`. Applying it is the same act as lifting the
   pause, and it belongs to whoever owns the registry. Its cost is storage: dropping `--keep 10`
   and moving 7d → 30d retains strictly more, over five repositories that republish many times a
   day.

• Files reviewed: 8/8 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

Test Results (shard 0)

262 tests   262 ✅  2m 15s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit 7d48482.

[github-actions]

Test Results (shard 1)

1 425 tests   1 425 ✅  1m 28s ⏱️
    2 suites      0 💤
    2 files        0 ❌

Results for commit 7d48482.

[github-actions]

Test Results (shard 2)

883 tests   883 ✅  3m 31s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit 7d48482.

[github-actions]

Test Results (shard 5)

    6 files      6 suites   4m 46s ⏱️
2 093 tests 2 093 ✅ 0 💤 0 ❌
2 097 runs  2 097 ✅ 0 💤 0 ❌

Results for commit 7d48482.

[github-actions]

Test Results (shard 3)

1 047 tests   1 045 ✅  6m 23s ⏱️
    3 suites      2 💤
    3 files        0 ❌

Results for commit 7d48482.

[github-actions]

Test Results (shard 4)

1 829 tests   1 637 ✅  4m 27s ⏱️
    4 suites    192 💤
    4 files        0 ❌

Results for commit 7d48482.

[github-actions]

Test Results

   18 files     18 suites   22m 52s ⏱️
7 539 tests 7 345 ✅ 194 💤 0 ❌
7 543 runs  7 349 ✅ 194 💤 0 ❌

Results for commit 7d48482.