Skip to content

review/land dispatchers leak an ambient gateway env onto the plain claude route #37

Description

@SSFSKIM

Problem & intent

review-dispatch.sh and land-dispatch.sh have the same gateway-env leak that
PR #35 just fixed in implement-dispatch.sh: the plain (claude) spawn branch
invokes daemon-spawn.sh with no environment prefix, so an ambient
DAEMON_CLAUDE_SETTINGS / DAEMON_CLAUDE_EFFORT is inherited. daemon-spawn.sh
then applies --settings/--effort AND persists both into the registry meta,
and daemon-resume.sh restores them on every later fork — so the worker rides
the clodex gateway for its whole life while the dispatch log says
engine=claude.

Discovered by the review worker on PR #35 (surfaced by the review engine on the
implement dispatcher, generalized by the reviewer). Confirmed mechanically on the
implement lane before the fix: with DAEMON_CLAUDE_SETTINGS=/tmp/x.json DAEMON_CLAUDE_EFFORT=high exported,
bash tests/implementing-tickets/test-implement-dispatch.sh failed its
"no gateway env" assertions with
spawn-env:settings=/tmp/x.json;effort=high. The same shape is present at:

  • skills/reviewing-prs/scripts/review-dispatch.sh — plain branch around the
    daemon-spawn.sh call (the gateway branch is at :407-408)
  • skills/reviewing-prs/scripts/land-dispatch.sh — plain branch around the
    daemon-spawn.sh call (the gateway branch is at :313-314)

Why it matters now rather than later: the review and land lanes still default to
codex, so today only an engine:claude label or WORKER_ENGINE=claude reaches
the leaking branch — narrow but real. #32 (C5) flips those two lanes to a
claude default, which makes the leaking branch the COMMON path, exactly as
happened on the implement lane in #33/#35. Fixing it before or as part of #32
avoids shipping the same silent-misrouting default twice.

Constraints

Success criteria

  • A plain-route review dispatch and a plain-route land dispatch each hand
    daemon-spawn.sh no gateway settings and no gateway effort, regardless of what
    the ambient environment exports.
  • A regression assertion proves it in tests/reviewing-prs/test-review-dispatch.sh
    and tests/reviewing-prs/test-land-dispatch.sh. The implement lane's approach
    is a good precedent: export an ambient gateway route for the whole suite, which
    converts every existing "no gateway env" assertion into a leak regression and
    simultaneously strengthens the gateway-route assertions into proof that the
    prefix overrides an ambient value rather than coinciding with an empty one.
  • The gateway route itself keeps working unchanged in both lanes.

Open questions

  • Should this be fixed at the substrate instead of per-dispatcher? Raised by the
    PR feat(implement): default model route flips clodex → claude (C6) #35 fixer as out-of-scope scope-creep: daemon-spawn.sh gives a caller no
    way to declare "plain route" other than passing empty strings — the contract is
    env inheritance, which is what makes this leak class available to every
    caller, including future ones. An explicit --no-gateway flag or a route
    argument would make the plain route unspoofable in one place rather than in
    three. Recommend: land the three-line dispatcher hygiene fix now (it is what
    C5: review-loop worker default route clodex → claude + plumbing removal #32 needs), and treat the substrate contract change as a separate design
    decision — it touches every daemon caller and wants its own gate.

Decision log

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingpriority:P2issue-tracker board prioritystatus:ready-for-agentissue-tracker board state

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions