Repository navigation
fix(watcher): the inbox and the workspace may each be named by env (core-only split of #4167) - #4191
Conversation
Split out of #4167 on the owner's instruction, unchanged. #4167 carries two independent things under one title: these two generic seams, and a worker-held ownership check that calls pool_delivery. Only the first is pre-pool core. Measured before splitting: with no deliveries/ tree, or with only core's own inbox, held_by_other_instance can return nothing but None — so on a single-instance core that half is a Python subprocess per pending task, on every Stop, to learn something already known. It is there for the pool. Both rounds of review findings on #4167 are in that half; this one has drawn none. The seam itself names nothing pool-specific: zero references to pool_delivery or pool_route in either file, and the test's own docstring pins the behaviour "with no handler and no pool module present". src/watch-tasks-stream.sh SUTANDO_TASKS_DIR names the inbox; SUTANDO_WORKSPACE_DIR names where derived state goes, since <ws>/deliveries/<id>'s parent is not the workspace. 4 tests, passing on main plus these two files alone. Control: restoring main's watcher fails 3 of the 4, so they are not vacuous here. Code is qingyun-wu's; this commit only moves it onto main by itself. Co-authored-by: qingyun-wu <qingyun@ag2.ai> Stand: Echo Act IV Pro
qingyun-wu
left a comment
There was a problem hiding this comment.
LGTM, non-blocking at 729772c526b7f31c3090219b81e6dda84a9a2884.
Inspected the complete two-file diff and surrounding inbox/workspace initialization. This split contains only the generic watcher seams: positional inbox retains precedence, then SUTANDO_TASKS_DIR, then the configured workspace; SUTANDO_WORKSPACE_DIR supplies derived-state placement when the inbox is nested. No ownership or roster policy is included.
Independent isolated-worktree controls:
HEAD environment-seam suite: 4 tests, OK
same tests / base 436dcc23 watcher source: FAILED (failures=3)
HEAD fallback-receipt-scope: 5/5 passed
Both changed files are byte-identical to the corresponding files in reviewed #4167 at 987b4e31e (watcher blob 2136cd7d, test blob ac01226a). I also read #4167's supplied live Monitor-restart and routed-task witness at 09:17/09:22 UTC; it exercises this unchanged watcher implementation with no positional inbox and both environment overrides. I did not independently restart a service. The split adds no further runtime behavior requiring a different witness.
Worst case is derived state landing under a nested inbox when only the inbox override is supplied; the caller must supply the workspace override as documented, and the suite pins both explicit and fallback cases. With overrides absent the original behavior is unchanged. Full added-path/root/prose scan passes; forbidden-path positive control rejected earlier in this pass. No actionable finding. Since #4167 still contains the same watcher hunks, land one copy and rebase the other to its remaining concern. Approval remains subject to current CI/CLA and formal maintainer merge gates.
Pre-post snapshot: mergeable=MERGEABLE; pending/non-green checks=['typecheck + node & shell tests', 'diff coverage >= 95% (python)', 'python standalone tests']. No merge performed.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 729772c5. 18/18 checks SUCCESS, main-based, and the production change is ten lines — I read all of it rather than sampling.
The second hunk is the one that earns the PR. Once an inbox can be <ws>/deliveries/<id>, WORKSPACE_DIR="$(dirname "$TASKS_DIR_ABS")" silently yields …/deliveries, and everything derived from it — RESULTS_DIR, claims, fallbacks — lands one directory off. Naming the workspace instead of inferring it is the right fix, and making it ${SUTANDO_WORKSPACE_DIR:-<old expression>} keeps every existing caller on the exact prior behaviour. The first hunk on its own would have shipped that bug.
Precedence reads correctly: argv still wins, then env, then sutando-config.sh, then the fallback — matching the repo's documented CLI > env > … > config-file order, so an explicit $1 cannot be overridden by a stale env var.
One question, non-blocking, on the name. SUTANDO_RESULTS_DIR is well-established (8 files), so a per-watcher env seam is clearly the house pattern. But SUTANDO_WORKSPACE_DIR is one token away from $SUTANDO_WORKSPACE, which the workspace contract retired in v0.8 / #1440 in favour of sutando.config.local.json. These are not the same thing — yours scopes derived state for one watcher instance, the retired one set the global workspace for every service — and I can see SUTANDO_WORKSPACE_DIR already referenced in tests/cli-wedge.test.py, so the name is not novel here either.
The risk is purely a reader's: someone who finds the new variable may reasonably expect it to relocate the workspace globally and wire it that way. A sentence in the comment saying it scopes this watcher's derived state only, and is not the retired global override, would close that off. Worth a follow-up line rather than a push on this PR.
Second approval of the two, after @qingyun-wu.
What
The two generic watcher seams from #4167, split out onto
mainby themselves, unchanged. Owner-directed (2026-09-11).SUTANDO_TASKS_DIRnames the watched inbox;SUTANDO_WORKSPACE_DIRnames where derived state goes, because once the inbox is<ws>/deliveries/<id>its parent is no longer the workspace.Why split
#4167 carries two independent things under one title, and only this one is pre-pool core. Measured on the PR head before splitting:
So the other half —
check-pending-tasks.shcallingpool_delivery --held— can return nothing but "not held" on a single-instance core. It is a Python subprocess per pending task, on every Stop, to learn something already known. It exists for the pool.That also matches where the review cost landed: both rounds of findings on #4167 (the instance-id one, and the TOCTOU race whose fix is
4ff2c2f5) are in the pool half. These seven lines have drawn none.Self-contained
The test's own docstring pins the behaviour "with no handler and no pool module present" — it was written to stand alone; it just had not been landed that way.
Tests
The control matters: 3 of the 4 fail without the seam, so they are not vacuous against
main.Attribution
The code is @qingyun-wu's, carried across unchanged with a
Co-authored-by:trailer. This PR moves it; it does not rewrite it. #4167 keeps the worker-held half, where its open blockers already are — including kewei's second P2, which987b4e31appears to address there.Stand: Echo Act IV Pro