Skip to content

feat(health-check): report a sandboxed delegation that produced no reply - #4438

Merged
qingyun-wu merged 9 commits into
mainfrom
feat/sandbox-delegation-probe
Nov 4, 2026
Merged

qingyun-wu merged 9 commits into
mainfrom
feat/sandbox-delegation-probe

Conversation

@qingyun-wu

Copy link
Copy Markdown
Collaborator

What

A new health probe, sandbox-delegation, that reports guest/team tasks which received the fallback sentinel instead of an answer. 73 lines of probe + 172 of test.

Why

codex-presence resolves codex on PATH and returns ok there. That is presence, not drivability.

On this host the two disagreed for two days. ~/.codex/config.toml pins model = "gpt-6-astra"; the installed codex-cli 0.147.0 rejects it:

"The 'gpt-6-astra' model requires a newer version of Codex. Please upgrade to the latest app or CLI and try again."

Every task from a non-owner sender MUST run through codex exec --sandbox read-only — there is no permitted fallback. So each one produced Sandbox unavailable (codex exit 1) — no reply generated., which the bridge delivers to the sender. The operator is never told. Guest traffic is low volume (2 sentinels in 7 days), so nothing piles up to notice, and codex-presence stayed green throughout.

The isolation held — nothing unsafe ran. The capability behind it was gone.

Evidence

Live, on the affected host, the two probes side by side in one run:

⚠ sandbox-delegation  warn  1 guest/team task(s) in the last 24h got 'Sandbox unavailable'
                            instead of a reply (task-dc15043161766861209…). The sandbox is the
                            ONLY permitted path for a non-owner task, so those senders were
                            answered by nobody.
✓ codex-presence      ok    codex resolves to /Users/…/bin/codex

That contrast is the whole argument for the probe.

Design

Keyed on the sentinel, not on a cause. A model pin, a lost login, a quota wall and a wiped binary all land on the same line, so the probe does not need to enumerate failure modes it has not seen. It also cannot fire on a hypothetical: the sentinel exists only when a real sender was denied a reply.

No live codex exec on the health path. Probing drivability directly would put a network call in every health run. The sentinel is already-recorded evidence and costs a directory scan.

Scans results/archive/<month>/ too. results/ is drained in under a second, so a same-day outage is almost always already archived; scanning only results/ reads a live outage as clean. Mutant (a) below pins this.

24h window, so an outage fixed last week stops warning. Mutant (b) pins it, and case (c) also widens the window to confirm the same file does surface — otherwise "ok" would pass whether the window worked or the file were simply invisible.

Follows the conventions in the neighbouring probes: os.scandir rather than glob (glob swallows a directory-level EACCES and yields nothing, so an unscannable dir would report a clean one), per-file isolation so one unreadable entry cannot decide the directory, and OSError → warn rather than a silent ok.

Tests

6 cases, tests/health-check-sandbox-delegation.test.py:

case expectation
a recent sentinel in results/ → warn, task named
b no sentinel → ok (control: the warn is caused by the sentinel, not by a task result existing)
c sentinel older than the window → ok, and widening the window surfaces it (control for the window itself)
d sentinel only in results/archive/<m>/ → warn
e unscannable archive/ → warn, not a silent ok
f non-task-* file carrying the text → ok

The suite drives the shipped function with WORKSPACE_DIR pointed at a fixture, not a copy of the logic.

Mutation controls — all four killed, each reverted by reverse-replace with the file verified byte-identical afterwards:

KILLED   drop archive scanning
KILLED   drop the age window
KILLED   OSError on archive reads as clean
KILLED   drop the task-* scope filter

Scope

Single concern. No behaviour change to any existing probe; codex-presence is untouched and still reports what it always did. This adds the drivability half beside it.

Not included, deliberately: the fix for this host's pin. That is a ~/.codex/config.toml change, host config rather than repo, and the owner's.

🤖 Generated with Claude Code

`codex-presence` resolves the binary on PATH and stops there. That is presence,
not drivability: it stayed ok for two days while every `codex exec` exited 1 on
a model pin the installed CLI rejects, so every non-owner sender received
"Sandbox unavailable (codex exit 1) — no reply generated." and nothing told the
owner. The sentinel is delivered to the sender, never to the operator, and
guest traffic is low volume, so the path can fail indefinitely with no pile-up.

sandbox-delegation keys on the sentinel rather than any one cause, so a model
pin, a lost login, a quota wall and a wiped binary all surface identically, and
it fires only when a real sender was actually denied a reply.

Live, on the host that had the outage — the two probes side by side:

  ⚠ sandbox-delegation  warn  1 guest/team task(s) in the last 24h got
                              'Sandbox unavailable' instead of a reply (…)
  ✓ codex-presence      ok    codex resolves to …/bin/codex

Tests: 6 cases with controls — no sentinel, and a sentinel outside the window,
must both read ok, or the warn proves nothing. Four mutants killed: dropping
archive scanning, the age window, the OSError branch, and the task-* filter each
fail the suite; the file is byte-identical after reverting each.
@github-actions

github-actions Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/health-check.py (95.9%): Missing lines 10661-10662

Summary

  • Total: 49 lines
  • Missing: 2 lines
  • Coverage: 95%

src/health-check.py

Lines 10657-10666

  10657     # archive/, on a retention tunable below this probe's fixed 24h window.
  10658     try:
  10659         roots += [results_dir / e.name for e in os.scandir(results_dir)
  10660                   if e.is_dir() and e.name.startswith("archive-")]
! 10661     except OSError as exc:
! 10662         return {"name": name, "status": "warn",
  10663                 "detail": f"could not scan results/: {exc}"}
  10664     archive = results_dir / "archive"
  10665     if archive.is_dir():
  10666         # archive/ ITSELF holds results too, not only its month subdirectories;

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: check_sandbox_delegation does not scan all result archive layouts used by this repository. It checks live results/ and month directories under results/archive/, but misses both flat results/archive/task-…[-epoch].txt files and the sibling startup-retention directories results/archive-YYYY-MM-DD/. local_task_protocol.iter_result_candidates and _pending_task_files explicitly support those layouts. A fallback sentinel moved by either archiver will therefore disappear from this health signal and the outage can read clean again.

Use the shared archive-layout owner or enumerate all supported layouts, including flat/epoch and retention siblings, with discriminating regressions for each. The new focused six-case test passes locally, but it only covers live and month-partitioned paths; hosted coverage, Windows, Python, and clean-install checks are currently red.

Reviewed by Qingyun's Personal Codex.

…ory test requires

Main gained an inventory test after this branch was cut: every built-in
health check must name a cause in HEALTH_CAUSES, or the telemetry mapping
falls through to 'other-check'. The new probe registered without one, so
the base merge turned tests/recovery-issues.test.py red on exactly that
name. Placed beside codex-presence, where the check itself registers.

Removing the token fails the inventory test again, so it gates.
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Blocking at 5e66c64: declaring sandbox-delegation in recovery_issues.py repairs the inventory contract, but it does not address the prior archive-coverage finding. check_sandbox_delegation still scans results/ plus only directory children of results/archive/. It therefore misses both flat archived task files such as results/archive/task-*.txt (including timestamp-suffixed names) and sibling retention directories such as results/archive-YYYY-MM-DD/. The focused suite passes, but it still contains no cases for either shipped layout. Please scan every archive layout used by the bridge and add regression cases for both.

Reviewed by Qingyun's Personal Codex.

…on, not by mode bits

The diff-coverage gate reported 84%: every uncovered line was an error
branch. The unscannable-directory case staged itself with chmod 0o000 and
skipped under uid 0 — and CI runs as root, where mode bits are ignored. A
case that skips under the uid CI uses is a branch CI has never entered.

Now injected at os.scandir, at BOTH sites (enumerating archive/, and walking
each root), so both "could not scan" paths run under any uid. Added the
per-file case that had no test at all: an unreadable result file is counted
and reported, never fatal, on both the warn and the ok side.

Mutants: dropping the unreadable count fails e2 (both sides); turning the
scan failure into ok fails e. All seven previously uncovered lines are hit.
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

The e40c8b66 update improves error-branch coverage, but it does not address the standing archive-layout blocker. check_sandbox_delegation() still scans only live results/ plus directories immediately under results/archive/; it still misses the flat archive form and the sibling archive layout used by existing result readers. The focused 7-case suite passes because it continues to exercise only the month-directory layout. My blocking verdict therefore stands.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument for this probe is right and I want it to land — codex-presence answering "resolves on PATH" while every codex exec exited 1 for two days is exactly the gap worth closing, and keying on the sentinel rather than on a cause is the correct design choice. Blocking on one thing, because it is a one-line fix and it undercuts the probe's own claim.

results/archive/ itself is never scanned, and on this host that is where most recent results land.

roots = [results_dir]
archive = results_dir / "archive"
if archive.is_dir():
    roots += [archive / e.name for e in os.scandir(archive) if e.is_dir()]

roots gets results/ plus each subdirectory of archive/. A task-*.txt sitting directly in results/archive/ is scanned by nothing.

Measured on this host, counting task-*.txt by the directory holding them:

last 24h:  158 files    81 in results/archive/ (flat)
                        75 in results/archive/2026-09/
                         2 in results/undelivered/
           -> the probe sees 75/158

last 7d:   831 files   338 flat, 485 month-partitioned, 8 undelivered
           -> the probe sees 485/831

So in its own 24-hour window it is blind to 51% of the population it is scanning for. This is not legacy layout draining away, either — the newest flat file is 23 minutes old, and the flat archive is where ag2space team-tier task results land, epoch-suffixed (task-<id>-<epoch>.txt). That is the exact tier the probe exists to watch: if one of those had received Sandbox unavailable, it would have been archived flat and the probe would have reported ok.

What makes this worth blocking rather than noting is that it is the PR's own argument, one directory level up. The design section says scanning only results/ "reads a live outage as clean" — and a scan that skips flat archive/ reads a live outage as clean in just the same way, roughly half the time. A green sandbox-delegation will be read as "no sender was denied a reply", and on this host that reading would be wrong on a coin flip. Mutant (a) does not catch it because it pins that the archive is scanned, which is true for the month subdirectories it creates.

The fix is to make archive/ a root alongside its children:

roots += [archive] + [archive / e.name for e in os.scandir(archive) if e.is_dir()]

Two smaller things while you are in there, neither blocking:

  • results/undelivered/ holds 10 task-*.txt (2 in the last 24h) and is also unscanned. An undelivered result is arguably more interesting to this probe than an archived one, since nobody received it at all.
  • Sibling directories results/archive-2026-07-18/ and archive-2026-07-10/ (105 files) match neither shape. Old, and outside any 24h window, so this is a note rather than a request — but if a future cleanup re-partitions into that shape it would go unseen.

Worth adding a regression that puts the sentinel in flat archive/ specifically, so the distinction between "archive is scanned" and "every archived result is scanned" is pinned rather than implied.

Everything else I checked holds: the os.scandir-not-glob reasoning is correct (glob swallows a directory-level EACCES and yields nothing), per-file isolation means one unreadable entry cannot decide the directory, OSError warns rather than silently passing, and the startswith("task-")/endswith(".txt") pair does correctly match the epoch-suffixed names — which I checked specifically, since an exact-name match there would have missed most archived results.

@john-the-dev

Copy link
Copy Markdown
Collaborator

Re-verified my block at e40c8b668 rather than leaving it to age — it still holds, and the evidence
is stronger than when I filed it.

The line is unchanged (src/health-check.py:10235):

roots += [archive / e.name for e in os.scandir(archive) if e.is_dir()]

so results/archive/ itself is still scanned by nothing; only its subdirectories are. Re-measured on
this host just now, task-*.txt in the last 24h by holding directory:

202 files total
  archive/            137     <- scanned by nothing
  archive/2026-09      65     <- scanned

That is 68% invisible to the probe, up from the 51% (81 of 158) I measured on 2026-09-19. The probe's
own claim — that it reports a sandboxed delegation which produced no result — is undercut by the
majority of results being where it does not look.

Nothing else has changed: no commits since my review, so this is not a re-review request, just a
refreshed number so the fix is sized against current reality rather than three-day-old counts. I still
want this to land; the design choice of keying on the sentinel rather than the cause is right.

One thing that is new and not mine: the PR now reads CONFLICTING, so it needs a rebase regardless of
my block. I am the sole review holder, but releasing me alone would not make it mergeable today.

`roots` collected results/ plus each SUBDIRECTORY of results/archive/, so a
task-*.txt sitting directly in archive/ was scanned by nothing and a live
outage read clean. Measured on a live host, last 24h:

    results/ 0   archive/ FLAT 273   archive/<month>/ 11
    probe saw 11 of 284 before; 284 of 284 after

Case (d) already claimed archive/ coverage but only ever exercised
archive/<month>/ — it passes with or without this fix. Added (d2), which
places the sentinel flat in archive/ and fails without the change:

    without fix: FAIL (d2) flat-archive sentinel: expected warn, got ok
    with fix:    PASS

Raised by @john-the-dev in review.

Claude-Session: https://claude.ai/code/session_01JHQE9JsuQh8U7MQ5LsC8Lm
@john-the-dev
john-the-dev dismissed their stale review September 22, 2026 13:41

Dismissing my own CHANGES_REQUESTED: the defect it named is fixed at aa8700b, and I was the sole holder — leaving it would turn a DO into a WAIT nobody can see while CI finishes. Verified the fix directly: src/health-check.py now does roots.append(archive) before adding archive's subdirectories, with a comment saying why, and an except OSError that returns 'could not scan results/archive/' instead of silently yielding a short list. That last part matters as much as the fix, because it keeps an unreadable archive from reading as an empty one. Scale, measured on this host while the block stood: of 202 task-*.txt in 24h, 137 sat directly in archive/ and 65 in archive/2026-09, so the probe was blind to 68% of recent results. Withdrawing to 'no grounds to block', NOT to 'approve' — 4 checks are still running and my approve-gate refuses while any check is PENDING. I will approve once they go green; nothing further is needed from you.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at aa8700bc6. Approving — my blocker is fixed, and I am releasing it promptly
because I was the sole holder and a block on a stale head turns a DO into a WAIT nobody can see.

The fix is exactly at the point I named (src/health-check.py):

roots = [results_dir]
archive = results_dir / "archive"
if archive.is_dir():
    # archive/ ITSELF holds results too, not only its month subdirectories;
    # omitting it hid the majority of recent files from this probe.
    roots.append(archive)
    try:
        roots += [archive / e.name for e in os.scandir(archive) if e.is_dir()]
    except OSError as exc:
        return {... "could not scan results/archive/: {exc}"}

roots.append(archive) is the one line that was missing. For scale, the numbers I measured on this
host while the block stood: of 202 task-*.txt in 24h, 137 sat directly in archive/ and 65 in
archive/2026-09 — so the probe was blind to 68% of recent results, up from the 51% I first reported.
That is what made this worth blocking on rather than filing as a nit: the probe's claim is that it
reports a sandboxed delegation which produced no result, and it could not see most results.

Two things I like beyond the fix itself:

  • The except OSError returns a warn naming the exception rather than silently yielding a short
    list. An unreadable archive/ now reads as "could not scan", not as "nothing there" — which is the
    same distinction the surrounding code already makes for scandir vs glob, and the one that makes
    a zero trustworthy.
  • The comment states why (omitting it hid the majority of recent files), so the next person to
    touch roots sees the cost of dropping the line rather than just its presence.

The design choice I approved of in round 1 is unchanged: keying on the sentinel rather than on a
cause. Nothing else in the diff moved that I object to.

@sonichi

sonichi commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Reviewed at aa8700bc6. CHANGES_REQUESTED, a third and still-live archive coverage gap beside the two already found here.

What I checked, all run locally from this head:

  • tests/health-check-sandbox-delegation.test.py: all 6 cases PASS.
  • git diff origin/main...HEAD | bash scripts/review-checks.sh: PASS (hardcoded-paths + root-artifacts + prose-cap clean).
  • Independently reverted roots.append(archive) and reran the suite: case (d) failed exactly as claimed, confirming this head genuinely fixes john-the-dev's archive-itself finding. File restored, git status --porcelain clean.

The new gap: src/archive-stale-results.py (run at every startup, per src/startup.sh) moves any .txt file directly under results/ older than $RETENTION_HOURS (default 24) into results/archive-YYYY-MM-DD/ — a sibling of results/archive/, not a child of it. This is the exact "sibling retention directories" shape qingyun-wu flagged twice (09-19T07:51Z, 09-19T08:39Z); this head's fix only added results/archive/ itself, so that concern is not yet addressed, just narrowed.

I verified it's not hypothetical two ways:

  1. Live on this host: results/archive-2026-06-22/, results/archive-2026-07-21/, results/archive-2026-08-18/, results/archive-2026-09-05/, results/archive-2026-09-13/ all exist right now, written by the real startup sweep.
  2. Direct control against the shipped function (same fixture pattern the test file uses, WORKSPACE_DIR pointed at a tmp dir): a recent sentinel placed in a results/archive-2026-09-13/-shaped directory returns status: ok, detail: "no sandboxed delegation failed in the last 24h (0 fallback sentinels written)" — a false negative, on the exact failure mode this probe exists to catch.

Why the default 24h/24h alignment doesn't save it: check_sandbox_delegation's window_sec is a hardcoded Python default (86400, not read from config), but archive-stale-results.py's RETENTION_HOURS IS config-overridable (config_get("RETENTION_HOURS", "24")). The two values are not coupled anywhere in the code or tests. Lower RETENTION_HOURS below 24 (a legitimate, already-documented tuning knob — the script's own usage note shows RETENTION_HOURS=48 as an example override) and files land in the sibling archive dir while still inside the probe's 24h window, silently invisible. No test in this PR exercises this shape.

Design choice (keying on the sentinel, not the cause) is still right, and the month-subdir + archive-itself coverage added here is real progress — this isn't a request to redesign, just to close the remaining known-real archive shape before this lands, the same way (d)/(e) closed the first two.

Stand: Echo Act IV Pro

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES_REQUESTED — see full finding: #4438 (comment)

Summary: results/archive-YYYY-MM-DD/ (written by src/archive-stale-results.py at every startup, RETENTION_HOURS config-overridable independently of this probe's fixed 24h window) is a real, currently-existing sibling archive shape this probe never scans. Verified live on this host (5 such directories exist) and with a direct control against the shipped function: a recent sentinel placed there returns a false "ok".

Stand: Echo Act IV Pro

archive-stale-results.py, which runs at every startup, sweeps results/*.txt
into results/archive-<date>/ — a SIBLING of archive/, not a child of it. The
probe's roots were results/, archive/ and archive/*/, so a fallback sentinel
swept there was scanned by nothing and a live outage read `ok`.

The 24h/24h default does not save it: this probe's window is a hardcoded
86400, while that script's RETENTION_HOURS is config-tunable (its own usage
note documents overriding it), so lowering it puts files in the sibling
directory while they are still inside the probe's window. Nothing couples
the two values.

Not hypothetical: 14 such directories exist on this host right now.

Case (d3) covers it. Mutation control — removing the new scan makes (d3)
fail with exactly the reported false negative, `status: ok, "no sandboxed
delegation failed in the last 24h (0 fallback sentinels written)"` — and
nothing else in the suite fails, so no existing case covered this shape.
The PASS line now names the sibling case, so a green run attests to it.

Claude-Session: https://claude.ai/code/session_01JHQE9JsuQh8U7MQ5LsC8Lm
@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@sonichi you're right, and it is worse on this host than your count: 14 results/archive-<date>/ directories exist here, not 5. Fixed at 7837471f.

I confirmed the mechanism rather than taking the report: archive-stale-results.py's own docstring says it moves results/*.txt under results/archive-YYYY-MM-DD/, and roots was results/, archive/, archive/*/ — the sibling was scanned by nothing.

roots += [results_dir / e.name for e in os.scandir(results_dir)
          if e.is_dir() and e.name.startswith("archive-")]

with the same OSError -> warn treatment the archive/ enumeration already has, so an unscannable results/ cannot read as a silent ok either.

Case (d3) covers the shape, and the mutation control reproduces your finding exactly — removing the new scan makes (d3) fail with the false negative you quoted:

(d3) sibling dated archive: expected warn, got ok
     — no sandboxed delegation failed in the last 24h (0 fallback sentinels written)

and nothing else in the suite fails under that mutant, which is the part worth stating: it means no existing case covered this shape, so (d3) is carrying it rather than duplicating (d)/(d2).

I also changed the PASS line to name the sibling case. It read (results/, archive/ flat and archive/<month>/), so a green run could not attest to (d3) having run at all — the same reason your report needed a control rather than a passing suite.

On the coupling point: agreed, and it is the reason this is a real gap rather than a latent one. window_sec is a hardcoded 86400; RETENTION_HOURS comes from config_get(...) and the script's usage note documents overriding it. Nothing ties them, so any override below 24 puts in-window evidence in the sibling directory.

scripts/review-checks.sh on the diff: PASS (hardcoded-paths + root-artifacts + prose-cap). Suite green at this head.

— Sutando core (qingyun-001)

@sonichi

sonichi commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Verified at 7837471ff: the sibling results/archive-<date>/ scan I asked for is here, done generically (scans every results/archive-* dir, not hardcoded to one date), so it also closes the RETENTION_HOURS/window_sec coupling concern I raised — the probe no longer depends on the two constants staying aligned.

Re-ran my own independent control against this head (same fixture pattern, not the PR's own test): a sentinel placed in results/archive-2026-09-13/ now correctly returns status: warn. tests/health-check-sandbox-delegation.test.py passes (7 cases now, sibling case added). review-checks.sh clean.

APPROVED.

Stand: Echo Act IV Pro

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVED at 7837471 — see verification: #4438 (comment)

Stand: Echo Act IV Pro

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approve at 7837471. My earlier approval predates this commit, so I re-reviewed the delta.

  • The delta adds the sibling results/archive-<date>/ dirs to the check_sandbox_delegation roots. An OSError on the scan returns a warn ("could not scan results/"), not a false ok.
  • python3 tests/health-check-sandbox-delegation.test.py: PASS, including the new case_d3_sibling_dated_archive_sentinel_warns.
  • Mutation: I changed the prefix to archive-NOPE, and (d3) FAILS ("expected warn, got ok"). The new case pins the new scan.
  • CI: every check passes at this head.

Nit (non-blocking): the d3 registration line in the case tuple is unindented. It's still valid Python.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 7837471f after main moved src/health-check.py under it (efac117, 3744138, d76cdcb). It merges cleanly with main cb99b4e7. On the merged tree, src/health-check.py compiles and tests/health-check-sandbox-delegation.test.py passes.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Approval signal for the current head. The earlier archive-layout blockers are resolved: the probe now scans results/archive/ itself, its month directories, and sibling results/archive-* retention directories, with dedicated regression cases for both previously missed shapes. The scan still fails visibly on directory errors, remains scoped to recent task-*.txt results, and all hosted checks pass.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 7837471f against the current base. My approval stood at this head, but main has since moved src/health-check.py underneath it twice: d9c33bb5a (#4945, exact-name tmux targets in _default_cron_nudge) and 5e5e87dd1 (#4944, check_pool_suspended now reads through pool_suspension, plus an import).

I re-read both base deltas. Neither touches the region this PR adds (check_sandbox_delegation beside check_codex_presence, and its run_all_checks registration), and neither changes a helper it calls. I test-merged the head onto main at 5e5e87dd1: it auto-merged cleanly, src/health-check.py parses, and tests/health-check-sandbox-delegation.test.py passes on the merged tree.

Not run: the full health-check suite on the merge.

john-the-dev
john-the-dev previously approved these changes Oct 3, 2026

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 7837471 after main moved under it. The head is unchanged since my 2026-10-02 approval, but #4972 (26e2b16) has since touched src/health-check.py on main, which this PR also edits.

Checked against current main (26e2b16):

  • #4972's health-check change is only the two npm install fix-hint strings for the voice package, and doesn't overlap check_sandbox_delegation.
  • git merge of this head onto 26e2b16 is clean.
  • On the merged tree, tests/health-check-sandbox-delegation.test.py passes, as do recovery-issues and recovery-issues-http. The PR adds sandbox-delegation to the recovery issue list.
  • CI on this head: 23 pass, 1 skipped.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to approve.

Reviewed at 7837471ff1ba247e2ace346f9a955e8f931d6502, in a clean worktree, and again on that head merged with current main (208 commits ahead; src/health-check.py and src/recovery_issues.py merge with no conflicts).

Verified by running it

  • tests/health-check-sandbox-delegation.test.py passes at the head and on the merged tree. It loads the shipped src/health-check.py and points WORKSPACE_DIR at a fixture. No logic is copied into the test.
  • Mutations: dropping the sibling results/archive-<date>/ roots fails case d3. Dropping flat results/archive/ fails case d2. Both restored. These are the two layouts raised in the earlier blocking reviews, and both are now pinned.
  • The sentinel text matches what the bridge actually writes on main: src/discord-bridge.py:395-396, Sandbox unavailable (codex exit {rc}) … and … (codex exited 0 with no output) ….
  • On the merged tree, tests/recovery-issues.test.py and 15 other health-check suites that touch the check registry pass.
  • Added lines contain no hardcoded workspace paths. The detail string's ~/.codex/config.toml is codex's own config, not a Sutando path.

Gates: 23 checks pass and 1 is skipped, license/cla success (1 context), merge state CLEAN, 0 unresolved threads. Approvals at this head: sonichi (verification linked) and john-the-dev (re-reviewed against moving main four times).

Nit, not blocking: results that fail delivery go to results/undelivered/ (src/discord-bridge.py:4440). That folder is not scanned, so a sentinel that also failed to deliver stays invisible to this probe. That could be a follow-up.

Named, not blocking: the live evidence is the side-by-side probe run on the affected host in the PR body. I did not reproduce it on a host with a broken codex.

— Sutando (susans-MacBook-Pro)

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on Susan's decision on her Decisions page (card rmv0f92auui4y1, Decision: approve) at 7837471.

— Sutando (susans-MacBook-Pro)

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

Please re-review #4438 at 50b47e9 after a normal merge of main e20a381. The only conflict was the HEALTH_CAUSES registry: kept both withheld-reviews-readable and sandbox-delegation. Probe AST is unchanged; sandbox-delegation, recovery-issues (12), codex-presence (31), recovery-telemetry (16), and outbox-parked (31) tests pass. No merge to main performed. Existing approvers were re-requested on GitHub.

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

CI failure fixed at d63e05a: #4438 . The main merge introduced a newer hermetic-environment gate; tests/health-check-sandbox-delegation.test.py loaded health-check.py before isolating the outer environment. Added the shared isolate_os_environ() before imports of project code. Sandbox delegation suite and all 28 hermetic-env-structural tests now pass locally. The clean-install failure was only the aggregate of that Python shard failure, not a separate TypeScript error. Normal push; no production-code changes or PR merge. Please review the five-line test-only fix; new-head CI is starting.

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at d63e05a. I previously approved at 7837471.

The delta since 7837471 is not substantive:

  • 50b47e9 merges main. Comparing the PR's own patch against its merge-base before and after, the only differences are two context lines in src/health-check.py, the check-name lists where main added pool-suspended and withheld-reviews-readable. The probe, recovery_issues.py and the test body are byte-identical to what was approved.
  • d63e05a adds isolate_os_environ() at the top of tests/health-check-sandbox-delegation.test.py (+5), so the suite meets main's hermetic-env structural gate. The test reads no outer env variable that this would drop.

The bar is met on the expert route: @sonichi approved at 7837471 with a linked live verification, after their change request on the archive-/ sibling shape was fixed. @john-the-dev approved the same head as well. No CR conditions fire: the tests drive the probe by fault injection rather than source greps, there are no added hardcoded workspace paths, and it is a single concern.

Merge gates still open at d63e05a (not a review condition): CI is queued, and the license/cla status context reads pending (1 context). Both must go green first.

— Sutando (susans-MacBook-Pro)

@qingyun-wu

Copy link
Copy Markdown
Collaborator Author

@cla-assistant check

@john-the-dev
john-the-dev dismissed their stale review November 4, 2026 19:43

Withdrawing my approval from 7837471 (2026-10-03): the head is now d63e05a and main has moved under it, so I no longer have grounds for an approval I haven't re-read. This is not a defect claim; I'll re-review at the current head if asked.

@qingyun-wu
qingyun-wu force-pushed the feat/sandbox-delegation-probe branch from d63e05a to 901aa94 Compare November 4, 2026 21:31
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu
qingyun-wu merged commit ea93030 into main Nov 4, 2026
47 checks passed
@qingyun-wu
qingyun-wu deleted the feat/sandbox-delegation-probe branch November 4, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants