Skip to content

Approvals: one role rule on every surface, decide only as root or the coordinator user, money paths read from main (#715) [PROTECTED: owner merges] - #805

Merged
jayden1711 merged 8 commits into
mainfrom
issue-715-approval-gate
Oct 19, 2026
Merged

jayden1711 merged 8 commits into
mainfrom
issue-715-approval-gate

Conversation

@jayden1711

@jayden1711 jayden1711 commented Oct 19, 2026 •

Copy link
Copy Markdown
Owner

Intent: One rule decides who may approve each kind of decision, and every surface uses it: a GitHub comment, a batched PR review, the dashboard, chat and the CLI. A high-risk merge (land) is the owners' only. Writers can approve one only when review.yaml on main says approvals: { land: writers } and the instance's policy.yaml writers_approve_land is on. That file is set by root or the coordinator user today, by root alone once #769 merges, and never from the dashboard. decide runs only as root or the coordinator user, and records the instance's owner. A batched review never merges while auto-merge is stopped, and the stop is checked again right before the merge call. The money-path list is read from main's tree, never from the coordinator's working tree, and the list file itself counts as a money path.

Risk class: protected. Every path under src/** is engine. This changes src/review.ts, src/decide.ts, src/dashboard-auth.ts (sign-in) and src/coordinator/merge-flow.ts (the merge policy), which are all self-modification parts. It also changes test/review.test.ts and docs/design.md, which are engine-guards and self-modification.

Closes #715

Money-path verification (done first, as the issue asked)

Run on origin/main 0a0aeda's build, in a scratch repo, with the coordinator's clone made and then only fetched (measured: node scratchpad/work/issue-715/verify-money.mjs):

1 self-edit PR (list shrunk + billing edit): L3 ["touches app code","money-path: src/billing/charge.js"]
   origin/main list: "src/billing/**\nsrc/payments/**"; checkout list: "src/billing/**"
2 stale list: payments change after main listed it: L1 ["touches app code"]
3 PR that edits only money-paths.txt: L0 ["only docs/tests, 2 lines"]
  • A PR can't change its own list before its review (measured). The list is read from the coordinator's working tree, and that tree is never at the PR head.
  • The real bug is that the list is frozen at clone time (measured). A path main added after the clone came out L1.
  • A list-only PR was L0 docs (measured). **/*.txt falls under docs.

All three are fixed here. The fail-without test below moves main after the clone, as the issue asked.

What changed

  • The one rule. src/dashboard-auth.ts mayAnswerKind(kind, role, approvals):
    • Owner decisions (plan, land, automation) are the owners'. The rest are the owners' and writers'.
    • land is also the writers' only with approvals.land === 'writers'. Any other value, near-misses included, means owners only (measured, below).
    • Callers:
      • the GitHub-comment path (src/coordinator/decisions.ts commentAnswer, for land, plan, domain and question);
      • the batched-review path (merge-flow.ts ownerApproved);
      • the dashboard (answerDecision, canAnswer and decision detail);
      • the CLI (through answerDecision);
      • Discord's merge approval (approveMerge). Discord is called with no approvals, so it stays owners-only, a narrower surface like voice. Chat replies never answer land (NEVER_FROM_CHAT, unchanged).
  • Where approvals comes from.
    • The coordinator uses its live config, which is main's review.yaml. It is filtered through writersApproveLand(), which reads the instance's policy.yaml on every approval. If that read is absent or throws, it is treated as off, so the change fails closed.
    • The dashboard uses decide.ts approvalsAt(root, origin/<branch>, policyLetsWriters). It reads main's config with git, caches it per config tree, and returns undefined for anything unreadable or invalid.
    • A local site (no policy) gets () => true, so its own review.yaml decides.
  • A GitHub comment counts only from a listed writer whose repo role passes the rule. The role comes from roleOf. Without roleOf, owners.default is the owner and the other listed writers are writers. If the lookup fails, the comment counts for nothing. The recorded answer now carries role.
  • decide.
    • decide.ts cliDecideRefusal runs before anything is read. It allows only root, the instance's coordinator user (wl-<name>, compared with the OS's own user name, never the environment), or, on a local site, the uid that owns the event log.
    • It then answers through answerDecision, as owners.default with role owner, via: 'cli', and the audit records os_user.
    • --instance is honoured now. Before, it was ignored and the cwd's log was written.
  • Batched review: !this.autoMergeStopped() is checked before ownerApproved (merge-flow.ts:574 after merging Merge: idempotent across a crash: merge.attempted before the API call, then reconciled, so the engine never reverts its own good merge (#733) [PROTECTED: owner merges] #793), as the Discord path does. It sits beside Merge: idempotent across a crash: merge.attempted before the API call, then reconciled, so the engine never reverts its own good merge (#733) [PROTECTED: owner merges] #793's triesCapped.
  • The one door (round 3): trainMerge checks emergencyInForce() || autoMergeStopped() immediately before mergeOurs (merge-flow.ts:452), after its own GitHub read. Every path into a merge goes through it. The earlier checks stay.
  • Money paths:
    • loadMoneyPaths(repo, src, rev) reads git show <rev>:<file>. The list file's own exact path is returned first.
    • Every coordinator caller goes through base.ts moneyPaths(), which reads refs/remotes/<remote>/<branch>. That covers all 15 call sites, landing.ts:94 included. The dashboard's decision detail reads origin/<branch>.
  • Config fallback (live.ts startConfig): if main's config is invalid or missing at start, the checkout's committed HEAD config runs (with the instance check), not a hand-edited working tree. The working tree is used only when HEAD has no valid config (a first run). The missing case was added in round 2.
  • Stop re-checked before the merge (round 2): mergeTested checks the emergency stop and the auto-merge stop again immediately before trainMerge. That covers the batched review and Discord's batched path.
  • Caller uid (round 2): decide takes the caller's uid from geteuid() through os/index.ts currentUid(), never from id on PATH.
  • Schema: review.yaml gets approvals: { land: owners|writers } (optional). policy.yaml gets writers_approve_land (default false). dashboard.audit.via gets cli.
  • Docs: design.md (what is read from main, the decisions rule, the review.yaml example), separate-users.md, the review.yaml template and the CHANGELOG upgrade notes.

No regression for existing installs (and what a writer who used to approve sees)

  • Nothing reopens (inferred from the diff; each path is tested below). Every surface only gets stricter by default. The single new loosening needs two explicit settings, one of them in the instance's own policy.yaml (never the dashboard's to set), and neither exists in any install today.
  • What changes for a writer who used to approve (in the CHANGELOG's Upgrading section):
    • Their /<cli> approve comment on a land decision or a held plan no longer answers it. Nothing is recorded and the decision waits for an admin.
    • A listed writer whose repo permission is read or triage now answers nothing by comment.
    • A batched review from an issue owner who isn't an admin no longer merges.
    • decide needs sudo -u wl-<name> ... --instance <name> (or root).
  • To keep writers approving land, the install sets both approvals: { land: writers } in review.yaml on main and writers_approve_land: true in policy.yaml. With only the review.yaml line, the setting is ignored, not refused. So turning the policy line off takes effect on the next approval and never stops an instance starting. (measured: letting writers approve… test)

How it was verified

Fail-without (measured). I ran the new tests against base 0a0aeda in a scratch worktree, with the new test files checked out over the old src/. All 14 new tests fail and the 12 existing batch tests pass. The key failures:

✖ GitHub comment: a writer's /<cli> approve doesn't land a held change when policy says owners only; an owner's does
  AssertionError: a writer approved nothing  actual: 1  expected: 0
✖ CLI: `decide` from an unprivileged user (not root, not the instance's coordinator user) is refused, and nothing is recorded
  AssertionError: recorded: d-cli -> approve (the coordinator acts on its next tick)
✖ money paths: a PR that edits the money-path list is judged by main's list, which moved after the clone
  AssertionError: never auto-merged: ...; doubt: no evaluator verdict; ...   (no money-path reason: the checkout's stale list)
✖ config at start: main's config invalid, the checkout's committed one runs, never a hand edit to its working tree
  actual: [ 'hand-edited' ]  expected: []
✖ batching: an owner's approving review merges nothing while auto-merge is stopped, as from Discord; ...
  AssertionError: an approval never overrides a stopped auto-merge
✖ batching: the issue's owner approving by review merges only if the one rule lets their repo role approve a high-risk merge
  AssertionError: a writer's review doesn't merge a high-risk PR
ℹ tests 26  ℹ pass 12  ℹ fail 14

Acceptance lines and the tests that fail without this change:

Acceptance line Test (all fail on 0a0aeda, pass here: measured)
a writer's GitHub comment can't approve a land decision when policy says owners only test/decision-gates.test.ts "GitHub comment: a writer's / approve doesn't land…"
decide from an unprivileged user is refused "CLI: decide from an unprivileged user… is refused, and nothing is recorded" (spawns the real CLI)
a PR that edits the money-path source file is judged by main's version "money paths: a PR that edits the money-path list is judged by main's list, which moved after the clone" (through mergeDecisionFor)
(comment scope) the batching path checks autoMergeStopped() test/pr-batch-coordinator.test.ts "batching: an owner's approving review merges nothing while auto-merge is stopped…"
(comment scope) config fallback "config at start: main's config invalid, the checkout's committed one runs…" and (round 2) "config at start: no config on main at all…"
(round 2) caller uid isn't steerable through PATH "CLI: decide by the user whose event log it is…", run with a fake id printing 4242 first on PATH
(round 2) stop re-checked right before the merge pr-batch-coordinator "batching: a stop raised while an approved merge is under way…"

Round-2 fail-without (measured on 875419a): I checked the two test files out over 875419a in a scratch worktree, built and ran them. tests 28, pass 25, fail 3, and the 3 are exactly the new cases:

✖ CLI: `decide` by the user whose event log it is records the instance's owner, the CLI and the OS user
  AssertionError: decide: refused: only root or the user whose event log it is may answer its decisions from the CLI, not jaydenv
✖ config at start: no config on main at all, the checkout's committed one runs, never a hand edit to its working tree
  actual: [ 'hand-edited' ]  expected: []
✖ batching: a stop raised while an approved merge is under way (during its checks) holds it, right before the merge call (#715)
  AssertionError: not merged: the stop came in before the merge call

After (measured, scratch HOME, state dirs and TMPDIR):

  • decision-gates 12/12 and pr-batch-coordinator 14/14 pass.
  • npm run typecheck is clean, src and web.
  • npm run check:denylist: "1289 files clean".
  • check-dco.mjs origin/main HEAD: "all commits signed off".
  • 56 related test files: 721 tests, 715 pass, 4 skipped, 2 fail. Those files include coordinator, dashboard, dashboard-auth, decision-detail, live-config, intake-coordinator, main-watch-coordinator, merge-policy, review, voice, siri-routes, automations-webhook and discord-chat.
    • The 2 failures are fix-forward-coordinator's two real-git tests. They fail the same way on base 0a0aeda in this sandbox (measured), so they are not caused by this change.
    • I didn't run the full suite locally: it includes the MCP broker tests, which the brief forbids running on this machine. CI runs it.

Changed existing test: test/review.test.ts "money paths load from the project source of truth" now commits the list and reads it at HEAD, because the API reads a ref. It keeps every assertion it had and adds one, that the list file itself comes first. Nothing was weakened.

Changed existing fixture (round 2): test/direct-land.test.ts now commits its instance's config.yaml edit, as a real instance's config is committed. Before, it relied on an uncommitted edit running when main has no config, which the round-2 fallback fix now refuses (CI run 39217174535 shows the four failures). Its assertions are unchanged.

Adversarial pass

Each attack, its result and what I changed because of it:

  • A writer's comment approving land or a held plan (default policy): refused. Measured, tests above.

  • A listed writer whose repo role is viewer, or whose role lookup fails (GitHub 502): answers nothing, owner comments included when the lookup fails. Measured, new test.

  • A writer pushes approvals: { land: writers } straight to main (an unprotected branch):

    • Before my change, this alone would have let writers in, which breaks "loosening is root-only".
    • Fixed: I added the instance policy's writers_approve_land, read on every approval.
    • Measured: off, a writer's comment lands nothing. Switched on, the same comment lands. Absent or throwing, it fails closed.
  • I first made writers-without-the-policy a config refusal. Turning the policy off could then stop an instance starting (loadInstance checks the working-tree config).

    • Fixed: the setting is ignored instead (policyViolations returns nothing for it). Measured in the test.
  • The coordinator built without the switch wired:

    • Before my change, an absent dep meant review.yaml alone decided, which fails open.
    • Fixed: absent means owners only. runSite wires () => true only for a local site (no policy.yaml).
    • Measured: no dep, a writer lands nothing.
  • Hand-editing the coordinator's checkout (review.yaml approvals, the money list, config.yaml at start): none of these takes effect. Measured: approvalsAt and the money and config tests.

  • decide as another user:

    • Refused. Measured by spawning the real CLI.
    • Spoofing USER, LOGNAME or SUDO_USER=wl-example is still refused, because the user comes from getpwuid. Measured.
    • An agent user on an instance, an empty user name, or an unknown uid: refused. Measured, probes.
  • decide OS calls:

    • The file-owner read goes through the OS layer (fileAccess).
    • Round 1 got the uid wrong: it came from userUid(user), which runs id -u found on PATH, so a fake id changed it (the reviewer measured this).
    • Fixed in round 2: the uid comes from currentUid() (process.geteuid()). Measured: with a fake id printing 4242 first on PATH, the log's owner is still recorded. On 875419a the same call was refused.
  • Steering the local log path (<PREFIX>_PROJECT_STATE_DIR, --root) in round 2: the owner check runs on the very file then written. Steering only picks which log is written, and passes only for a log the caller already owns and could write anyway. An instance's log is its coordinator user's, so it is refused (inferred from the code; the instance branch is name-checked and the test measures it).

  • Round 2: main's config dir deleted after the clone, plus a hand edit to the checkout: before the fix, the hand edit ran (['hand-edited'], measured on 875419a). Fixed: committed HEAD runs. Measured.

  • Round 2: a stop raised while an approved batched merge reads its checks: before the fix it merged (measured on 875419a). Fixed: a re-check right before trainMerge. Measured.

  • Near-miss policy values ('writers ', 'WRITERS') or the automation kind with land: writers: owners only. Measured, probes. The schema also rejects the near-misses (inferred, zod enum).

  • approvalsAt on a non-repo, or on a ref that doesn't exist: undefined, which means owners only. Measured, probes.

  • A money list path outside the repo (../outside.txt): git refuses it. A list missing on main throws, as a missing file did before. Measured, probes.

  • A batched review while auto-merge is stopped: no merge. Once the stop is cleared it merges. Measured.

  • A batched review by the issue's owner who is only a writer: no merge. With both settings it merges. Measured.

  • Turning things off: the policy line off applies at the next approval (measured). The auto-merge stop holds approvals on both approval paths (measured).

  • Round 3: a stop raised while the combined gate runs on the autonomous, non-batched path.

    • Before the fix, it merged (measured on ce43c04: not merged: the stop came in before the merge call fails).
    • Fixed: the check now sits at trainMerge's merge call.
    • The new test passes. Its control, with no stop raised, still merges (measured).
  • Round 3: every call into mergeOurs (measured by grep at f485333). The only caller of backlog.mergePr outside the backlogs is mergeOurs (merge-flow.ts:393). The only caller of mergeOurs is trainMerge (merge-flow.ts:455), right after the new check at :452. The callers of trainMerge:

    • The autonomous, non-batched merge (mergeOrWait, :624, after combinedGate): through :452. Measured by the new test.
    • mergeTested (:660; batched auto, batched review, and Discord's batched approval through mergeApproved): through its own :659 check and :452. Measured by the round-2 test.
    • mergeApproved (:725; Discord, not batched): through its own :724 check and :452 (inferred: no new test for this path; the same code sits in front of it).
  • Round 3: the start note now names which config ran: "started with the checkout's committed config (HEAD)" or "…working tree (no valid config committed at HEAD)". Measured: the two config tests assert the HEAD form.

What this can never do

  • Let a writer approve a high-risk merge or a held plan on any surface without both review.yaml on main and the instance's policy.yaml saying so. (tests above)
  • Let anyone but root or the coordinator user answer from the CLI (on a local site, root or the user whose event log it is), or record a CLI answer as anyone but the instance's owner. (tests above)
  • Merge a batched PR on an approval while auto-merge is stopped, including a stop raised while the coordinator prepares the merge, up to the check immediately before the merge call. (tests above) Not covered: a stop raised during the backlog's own token or rate-limit wait (up to about 10 s) or while the merge request is in flight; see Left open (inferred, from the round-3 final check, issuecomment-6263769924).
  • Judge a change by a money list or a review.yaml in the coordinator's working tree, or start on a hand-edited config when main's is invalid or missing and a valid one is committed. (tests above)

Compromised component

  • Agent user:
    • Can't run decide (measured: refused).
    • Can't change main's review.yaml or the money list except through a PR. Both are L3: review.yaml is self-modification, and the money list is now a money path (measured).
    • Residual (inferred): if the agent user could write the instance's events.db directly (sqlite, not the CLI), it could append a forged decision.answered. The CLI gate doesn't cover that. File modes do: the db is the coordinator user's (inferred, not re-measured here).
  • Coordinator user:
  • Dashboard:
    • Reads approvals from main and the policy's switch on each request. It can't loosen either one: writers_approve_land isn't one of the policy keys the dashboard form may change (measured by reading src/policy-edit.ts POLICY_KEYS).
  • A writer: loses land and plan approval by comment and by batched review. Gains nothing unless both settings are set.
  • Someone with push access to main: can set review.yaml approvals, but without the instance's policy line it has no effect (measured). They could change the money list on main, as before. Such a change is now L3 when it comes through the harness.
  • Root: can set writers_approve_land, today alongside the coordinator user, and alone once Single machine: policy.yaml is root's, and the engine refuses one that isn't (#726 part 1) [PROTECTED: owner merges] #769 merges.
  • Other instances / hub viewer: no new route.

Overlaps (measured, git merge-tree against every open PR head)

Left open

  • A stop raised during backlog/github.ts's token or rate-limit wait before the merge request (up to about 10 s), or while that request is in flight, can't hold that one merge. The in-flight part can't be avoided; the wait could be covered by a check passed into mergePr (a backlog-interface change, outside Approvals: the same role gate on every surface (GitHub comment, dashboard, CLI), and money paths read from main #715). Not a regression: on main the open window was the whole combined gate (inferred, round-3 final check).
  • Direct-mode landing ignores the auto-merge stop. The coordinator is filing this as its own issue. It's not in this PR.
  • Native Windows (guess): decide on a local site is refused there, because there's no uid to compare (fail closed). CI doesn't test Windows.
  • fix-forward-coordinator's two real-git tests fail in my sandbox on base 0a0aeda too (measured, same "both PRs open" assertion). This is environmental here, not caused by this change. CI is the judge.
  • Size: 585 insertions, 308 of them tests. One PR, because the issue's done_when names one test file.

New dependencies

None.

…s read from main (#715)

Who may approve a held change differed by surface: a listed writer's `/<cli> approve` comment landed a high-risk
(`land`) decision or a held plan that the dashboard keeps for owners; `<cli> decide` appended decision.answered with
no role check at all; a batched PR merged on its issue owner's review whatever their repo role, and while auto-merge
was stopped. And the money-path list was read from the coordinator's working tree, which is as old as its clone, so
a path main added later was L1, and a PR editing only the list was L0 docs.

- mayAnswerKind (src/dashboard-auth.ts) is the one rule; the GitHub comment path, the batched-review path, the
  dashboard and the chat path all call it. `land` is the owners' unless review.yaml on the default branch says
  `approvals: { land: writers }`, and an instance's root-owned policy.yaml `writers_approve_land` allows it. The
  switch is read on every approval; off, the setting is ignored (never a config refusal, so turning it off can't stop
  an instance starting).
- `decide` refuses anyone but root or the coordinator user before reading anything, and records the answer as the
  instance's owner through answerDecision, audited via `cli` with the OS user.
- The batched-review approval checks autoMergeStopped(), as the Discord path does.
- loadMoneyPaths reads the list at <remote>/<branch> with git; the list file itself is a money path. Every caller
  goes through one coordinator helper (base.ts moneyPaths) or the dashboard's main ref.
- startConfig, with main's config invalid, falls back to the checkout's committed config, not its working tree.
- Docs (design.md, separate-users.md, the review.yaml template) and CHANGELOG upgrade notes for writers who used to
  approve: they keep it only through the explicit review.yaml and policy.yaml settings.

Test (fails without): test/decision-gates.test.ts (writer comment, CLI as another user, money list moved after the
clone, config fallback) and test/pr-batch-coordinator.test.ts (stopped auto-merge, writer review).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (round 1)

Reviewed head 875419a1 against #715, judged by the decision guide. The design is right and every surface goes through mayAnswerKind. Three things need fixing: one identity read in the CLI gate comes from PATH, one branch of the start-config fallback still runs the working tree, and the "root-owned policy.yaml" wording isn't true yet.

Acceptance criteria

Criterion Result Evidence
One function decides who may approve each kind, and every surface calls it met (measured by grep, inferred by reading) The only writers of decision.answered are decide.ts:95 (answerDecision: dashboard, voice, chat via channel.ts:331, CLI via cli.ts:1095) and decisions.ts:147/183/282 (all through commentAnswer, which calls mayAnswerKind). Merge-on-approval paths are mergeApproved (Discord, through approveMerge, which calls mayAnswerKind('land', role) with no approvals, so owners only) and the batched ownerApproved (merge-flow.ts:668, which calls mayAnswerKind('land', …, this.approvals())). adminApproved (merge-flow.ts:216) only enqueues for a test batch and never merges.
land defaults to owners only; writers only through explicit policy met (measured) decision-gates "the one rule…", the "GitHub comment…" tests and the "letting writers…" test pass on the head. A switch that is absent or throws means owners only.
A writer's GitHub comment can't approve land when policy says owners only (fail-without) met (measured) On origin/main c339175a it fails: AssertionError: a writer approved nothing actual: 1 expected: 0. It passes on the head.
decide from an unprivileged user is refused (fail-without) partially met (measured) On origin/main it fails: recorded: d-cli -> approve. It passes on the head. The instance branch takes the user name from userInfo(). But the local-site branch takes the caller's uid from id found on PATH (required change 1).
The CLI records who approved, as the instance's owner met (measured) The test "…records the instance's owner, the CLI and the OS user" passes. The audit has via: cli and os_user.
A PR that edits the money-path source file is judged by main's version (fail-without) met (measured) On origin/main it fails: never auto-merged: … (no money-path reason). It passes on the head. My probes are below.
Money paths read from main at every caller met (measured) loadMoneyPaths now requires rev (typed), so a call without a ref doesn't compile, which makes the wrong thing impossible. The remaining calls are base.ts:277 (this.moneyPaths(), 16 call sites) and decision-detail.ts:170 (mainRef). On origin/main there were 17 calls.
Batched review checks autoMergeStopped() (owner ruling) (fail-without) met (measured) On origin/main it fails: an approval never overrides a stopped auto-merge. It passes on the head.
Config fallback (issue: live.ts:157-175) partially met (measured) The invalid-main branch now uses committed HEAD. The no-config-on-main branch (live.ts:165) still runs the working tree (required change 2).
Existing installs: nothing reopens; writers keep land only through explicit settings, and the change says so met (inferred from the diff) Every surface is tighter or unchanged by default. The new keys don't exist in any install today. CHANGELOG lines 9-14 and 22 describe what a writer sees. Its "root-owned" wording is wrong (required change 3).

Fail-without (measured)

I checked out the PR's three test files over origin/main c339175a in a scratch worktree, built (tsc emits despite the expected type errors for the new exports) and ran:
node --import ./dist/test/isolate.js --test dist/test/decision-gates.test.js dist/test/pr-batch-coordinator.test.js dist/test/review.test.js
Result: tests 45, pass 30, fail 15. That is the 14 new tests, plus the changed review.test.ts case, which fails on the list file itself first. Each fails on its assertion (three are is not a function for new exports: approvalsAt, cliDecideRefusal). The 12 existing batch tests pass. On the head: 45/45 pass. npm run typecheck exits 0 and npm run check:denylist reports "1289 files clean" (both measured on the head).

Money-path probes on the head build (measured; a scratch repo in the scratchpad, loadMoneyPaths(clone, src, 'refs/remotes/origin/main'))

The builder's verify-money.mjs, re-run on origin/main, reproduces the bug: self-edit L3, stale list L1, list-only L0. On the head:

  • self-edit (list plus billing): L3 money-path: money-paths.txt, src/billing/charge.js.
  • stale list (payments added on main after the clone): L3 money-path: src/payments/pay.js.
  • list-only PR, and the same with file: ./money-paths.txt: L3 money-path: money-paths.txt.
  • PR deleting the list: L3. Renames count by their old path because change-files runs with --no-renames (inferred from change-files.ts:9-12).
  • ../outside.txt: throws "outside repository". /etc/hostname: throws "not in refs/remotes/origin/main". --version: throws (no option injection, since the argument is rev:path).
  • Remote ref missing: throws. List deleted on main: throws. That is the same as a missing file before. Most callers let the throw propagate. protectedSurface treats it as protected, and intake treats it as none (pre-existing).
  • A main list of **: every change is L3. That direction is stricter, not looser.

Two-key design (principle 6)

Enabling writers is a loosening that only root or the coordinator user can make. It is not root-only yet (measured and inferred):

CLI decide attacks

  • USER, LOGNAME, SUDO_USER spoofing: refused (measured, the PR's test; the name comes from userInfo()).
  • sudo -u wl-<name>: no shipped sudoers rule lets anyone run as a coordinator user (measured, grep of scripts/). The only coordinator rules are instance.sh:40 (the coordinator to its agent user) and the root helpers. An admin with full sudo is effectively root.
  • setuid wrapper: only wl-<name> or root can make one owned by wl-<name>, and isRoot() reads the real uid (inferred).
  • Symlinked events.db: fileAccess uses statSync, so it checks the target's owner (inferred from os/index.ts:362).
  • Windows, or no uid: userUid returns null and fileAccess returns unsupported, so it is refused (inferred). It fails closed.
  • Fake id on PATH: the caller's uid changes (measured). With a fake id that prints 4242 first on PATH, the event log's own owner is refused: decide: refused: only root or the user whose event log it is …, not jaydenv. With the real id the same call is recorded. The uid comes from userUid(user) (cli.ts:1081), which runs id -u found through PATH. So a fake id printing another user's uid passes the local branch for that user's log (inferred). logPath itself follows WORKLANE_PROJECT_STATE_DIR (guardrails/context.ts:22), so the local branch can be pointed at any events.db, an instance's included. The practical gain is limited to someone who can already write that db (inferred). Still, the gate's identity comes from the environment, which the PR body says it never does.

Stop handling (principle 2)

  • The batched path now checks the stop (measured). The Discord path already did.
  • Direct-mode landing ignoring the stop: a follow-up, not required here (inferred).
    • Only PR mode writes the stop. The main watch is off in direct mode (merge-flow.ts:978), and fix-forward starts only after an auto-merge's merge.done (merge-flow.ts:818).
    • Direct landing never honours the stop at all, for autonomous L0/L1 landings as well as approvals. An approval there doesn't "override" a stop the mode never had. Changing that is a direct-mode change, larger than Approvals: the same role gate on every surface (GitHub comment, dashboard, CLI), and money paths read from main #715.
    • It still matters under principle 2: a stop left over from PR mode survives a land_mode switch, and land_mode is a dashboard POLICY_KEYS entry. File it as an issue.

Config fallback (item 5)

Running committed HEAD is a tightening: a hand edit to the checkout no longer runs when main's config is invalid (measured, the PR's test). It never blocks start, because every new path is in a try/catch that falls back as before (inferred from live.ts:169-181). But see required change 2.

Protection (measured)

Other checks

Required changes

  1. decide: take the caller's uid from the OS, not from id on PATH. cli.ts:1081 uses userUid(user), which runs id -u through PATH. Add an OS-layer currentUid() (process.geteuid(), null on Windows) and use it for caller.uid. Add a test that puts a fake id first on PATH and shows it changes nothing.
  2. startConfig: the no-config-on-main branch (live.ts:165) must not run the working tree either. Measured: with .worklane/ deleted on main after the clone and required_checks: [hand-edited] written into the checkout, the coordinator started with ["hand-edited"] (committed was []). The issue's live.ts:157-175 covers both branches, so fix the class: try committed HEAD first there too, and add a fail-without test. This also makes "What this can never do" Fix a lock race; cap running agents by count across instances #4 true.
  3. Correct the "root-owned / root's policy.yaml" claims to what holds today. These are CHANGELOG line 20 ("its root-owned policy.yaml") and line 36 ("root's policy.yaml"), instance.ts:149 ("Root's to turn on"), service.ts:113, decide.ts:23, and the PR body's "Compromised component → Coordinator user" bullet. Today: add-project.sh:222 creates it as the coordinator user, so root or the coordinator user can set it, and the dashboard form can't. Either reword it, or say the PR merges after Single machine: policy.yaml is root's, and the engine refuses one that isn't (#726 part 1) [PROTECTED: owner merges] #769 and why. While you're there, drop the "Left open" Single machine: policy.yaml is root's, and the engine refuses one that isn't (#726 part 1) [PROTECTED: owner merges] #769 item: Single machine: policy.yaml is root's, and the engine refuses one that isn't (#726 part 1) [PROTECTED: owner merges] #769 changes readPolicy itself, so no switch is needed.

Follow-ups (not blocking)

  • Direct-mode landing ignores the auto-merge stop entirely (see above). Needs an issue under principle 2.
  • The stop is checked when an approval is first seen, not again just before trainMerge inside mergeTested (batched and Discord-batched). A stop raised during the head-checks or fetch awaits isn't seen (inferred). The non-batched Discord path does re-check (merge-flow.ts:655).
  • Residual: a direct sqlite write to events.db can forge decision.answered. File modes are the only guard (the builder noted this).

New trade-offs for the owner

None. The second key (root's or the coordinator user's writers_approve_land) is stricter than the issue asked, and every default is a tightening.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…p re-checked before merge (#715)

Review round 1 found three gaps:
- `decide` took the caller's uid from `id -u` found through PATH, so a fake `id` could change who the gate thought
  was calling. It now comes from geteuid() through an OS-layer helper (os/index.ts currentUid). The local-site owner
  check stays on the very file then written, so steering logPath can only pick a log the caller already owns.
- startConfig's no-config-on-main branch still ran the checkout's working tree. Both fallback branches now try the
  committed HEAD config first; the working tree runs only when nothing valid is committed (a first run).
- mergeTested checked the auto-merge stop only when the approval was first seen. A stop raised during its checks or
  fetch now holds the merge right before trainMerge, for the batched review and Discord's batched path alike.

Wording: the instance's policy.yaml is root's or the coordinator user's file today, root's alone once #769 lands,
and never settable from the dashboard; the comments and CHANGELOG say so instead of "root-owned".

Test (fails without): test/decision-gates.test.ts (a fake `id` first on PATH; no config on main) and
test/pr-batch-coordinator.test.ts (a stop raised during an approved merge's checks).

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 2: changes for the round-1 verdict

Head 24c81b0b. It merges origin/main c339175a (merge commit 4b836b74, signed off, no conflicts) and adds one signed-off fix commit.

  1. decide's uid comes from the kernel (measured).
    • The caller's uid now comes from os/index.ts currentUid(), which is process.geteuid() (null on Windows). It used to be userUid(user), which runs id -u from PATH (cli.ts).
    • The "decide by the user whose event log it is" test now runs the real CLI with a fake id that prints 4242 first on PATH. It is recorded on this head. On 875419a the same call is refused: decide: refused: only root or the user whose event log it is … not jaydenv.
    • Why steering the log path doesn't matter (inferred from the code): the local-site owner check is on the very file that is then written (logPath(root), stat'd and opened by the same path).
      • Pointing <PREFIX>_PROJECT_STATE_DIR or --root elsewhere only picks which log gets written. The check passes only for a log the caller already owns and could write anyway.
      • An instance's log is its coordinator user's, and the instance branch is checked by name.
      • I wrote this in an INVARIANT: comment at the call site.
  2. Config fallback when main has no config directory (measured).
    • startConfig now tries committed HEAD first in both fallback branches (one helper). The working tree runs only when nothing valid is committed, as on a first run.
    • New test: .worklane/ is deleted on main after the clone and the checkout is hand-edited to required_checks: [hand-edited]. On this head the committed config runs. On 875419a it ran ['hand-edited'].
  3. Wording.
  4. Stop re-checked right before trainMerge (measured).
    • mergeTested checks emergencyInForce() || autoMergeStopped() immediately before trainMerge. That covers the batched review and Discord's batched path, since mergeApproved reaches mergeTested as well.
    • New test: a stop written while mergeTested reads the head's checks holds the merge. On 875419a it merged: AssertionError: not merged: the stop came in before the merge call.
    • Direct-mode landing is left to the separate issue.

Fail-without on 875419a (measured): I checked out both test files over 875419a in a scratch worktree, built and ran them. Result: tests 28, pass 25, fail 3. The 3 failures are exactly the new cases above.

On this head (measured; scratch HOME, state dirs and TMPDIR):

  • decision-gates, pr-batch-coordinator, live-config, first-run-live-config and brand: 40/40 pass.
  • typecheck (src and web) passes.
  • check:denylist: "1293 files clean".
  • check-dco origin/main HEAD: all signed off.

Adversarial pass (round 2):

  • A fake id on PATH can no longer change the caller's uid.
  • Steering the log path only reaches logs the caller owns (reasoned above).
  • A missing config on main plus a hand edit no longer runs the hand edit.
  • A stop raised mid-merge holds the merge.

Each is measured except the steering case, which is inferred. CI on 24c81b0b is still running and will be reported once it finishes.

…now runs the committed one (#715)

CI shard 4 on 24c81b0 failed the four start-up tests: their example instance has no origin config and edited
config.yaml (land_mode) in the working tree without committing it. Since round 2, with no config on the default
branch, the coordinator starts on the checkout's committed config, never an uncommitted edit, so it started in pr
mode. The fixture now commits the edit, as a real instance's config is; every assertion is unchanged.

Test (fails without): test/direct-land.test.ts's four start-up tests fail on 24c81b0 with the old fixture.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 2: CI, and one fixture fix

CI on 24c81b0b failed in test shard 4/4 on both Node versions (measured, run 39217174535). Four test/direct-land.test.ts start-up tests failed with no .worklane/ at origin/main: started with the checkout's … 0 !== 1.

  • Cause (measured): the test's example instance has no config on origin and set land_mode by editing config.yaml in the working tree without committing it.
  • Since round-2 fix 2, the coordinator starts on the checkout's committed config when main has none. So the coordinator started in pr mode, and the tests' direct-mode refusals didn't happen.
  • This is the fix doing exactly what the reviewer asked for.

Fix (ce43c049): the fixture commits its config edit, as a real instance's config is committed. Every assertion is unchanged, so no test was weakened under rule 10.

  • The fail-without for this fixture is that same CI run: the four tests fail on 24c81b0b with the old fixture.
  • With the new fixture, all 8 direct-land tests pass locally and in CI (measured).

CI on ce43c049: all pass (measured, gh pr checks 805). That covers:

  • both test matrices and all 8 shards;
  • dco, denylist, push-gate, scan, scope, new-deps, agentshield and desktop;
  • wizard e2e, add-project, egress firewall, test-run sandbox and the Claude sign-in probe (the real-systemd jobs).

Bootstrap was skipped as usual.

Local: typecheck, check:denylist ("1293 files clean") and check-dco origin/main HEAD all pass.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: CHANGES NEEDED (round 2)

Reviewed head ce43c049 against #715 and the round-1 verdict, judged by the decision guide. All three round-1 required changes are fixed, and so is the round-1 follow-up (the stop re-check in mergeTested). Two things block the merge:

Round-1 required changes

# Required change Result Evidence
1 decide's uid from the OS, not id on PATH met (measured) cli.ts:1084 uses currentUid() (os/index.ts:297, process.geteuid()). My probe put a fake id printing 4242 first on PATH, and the log's owner ran decide. Result: status=0 recorded: d-cli -> approve. On 875419a1 the PR's same test fails: decide: refused: only root or the user whose event log it is … not jaydenv.
2 startConfig: committed HEAD first in both branches met (measured) live.ts:165-185: one checkout() helper serves both branches. Probe results are below.
3 "root-owned / root's policy.yaml" wording met (measured by grep of the added lines) CHANGELOG, instance.ts, service.ts, decide.ts, base.ts, dashboard.ts and the PR body now say "root or the coordinator user today, root alone once #769, never the dashboard". Two test assertion messages still say "root" ('off unless root turns it on', "an instance whose root policy doesn't let writers"). That is a nit and doesn't block.
(follow-up) Stop re-checked right before trainMerge in mergeTested met for that path (measured) merge-flow.ts:594. The PR's test fails on 875419a1 (not merged: the stop came in before the merge call) and passes here. Required change 2 below covers the other paths.

Fail-without (measured)

I checked out test/decision-gates.test.ts and test/pr-batch-coordinator.test.ts from ce43c049 over 875419a1 in a scratch worktree, built and ran:
node --import ./dist/test/isolate.js --test dist/test/decision-gates.test.js dist/test/pr-batch-coordinator.test.js
Result: tests 28, pass 25, fail 3, which matches the builder. The three failures are exactly the new cases:

  • decide … records the instance's owner…: refused, not jaydenv.
  • config at start: no config on main at all…: actual ['hand-edited'], expected [].
  • batching: a stop raised while an approved merge is under way…: not merged: the stop came in before the merge call.

On the head, decision-gates, pr-batch-coordinator, direct-land, review, live-config and first-run-live-config gave 65/65 pass (measured). npm run typecheck is clean. check:denylist reports "1293 files clean". check-dco origin/main ce43c049 reports "all commits signed off" (all measured).

1. decide uid, and steering the log path (measured unless marked)

I ran a probe that spawns the real CLI built from the head, inside the isolate's temporary machine:

Attack Result
Fake id (prints 4242) first on PATH, caller owns the log recorded (the uid is geteuid)
WORKLANE_PROJECT_STATE_DIR → a dir whose events.db is a symlink to a root-owned file, with a fake id printing 0 refused (statSync follows the link; the target is root's)
State dir → a symlink to /etc (no events.db) refused (missing means fail closed)
State dir → a symlinked dir whose events.db links to a root-owned file refused
State dir → a symlink to a dir the caller owns passes the gate and writes only that log
--instance x with USER, LOGNAME and SUDO_USER set to wl-x refused (the name comes from getpwuid)

The INVARIANT holds (inferred, beyond the probes):

  • A swap after the check: a caller could swap their own events.db for a symlink to another instance's log between fileAccess and new EventLog. That gains nothing. The CLI runs as the caller's own uid with no setuid, so the kernel refuses to open a log the caller can't write. A log the caller can write, they could already write with sqlite3.
  • A caller-owned log that the coordinator then reads: an instance's coordinator reads <instance home>/state/events.db, in the coordinator user's 0700 home (add-project.sh), and the caller can't place a file there. instancesDir() follows WORKLANE_INSTANCES_DIR, but the --instance branch passes only for root or wl-<name> by name, and those can only write what they already own.
  • Residual, not a regression: on a local site, the coordinator, its agents (which inherit <PREFIX>_PROJECT_STATE_DIR, runner.ts:594) and the log's owner are one uid, so the local branch passes for all of them. On main anyone could run decide, and file modes can't separate processes running as the same uid.

2. Config fallback (measured, startConfig probe on the head, then on 875419a1, whose no-config branch is main's)

Case Head Before (main's code)
A: unborn HEAD, no origin, working tree only working tree working tree
B: .worklane/ untracked, no origin working tree working tree
C: origin with no config, .worklane/ untracked (the auto-config PR not merged yet) working tree working tree
D: config committed, no origin, working tree hand-edited committed working tree
E: committed config invalid, working tree valid working tree working tree
F: main valid, working tree hand-edited main's main's
G: main dropped .worklane/ after the clone, hand edit committed working tree

It never blocks a legitimate first run (measured for A-C and E; inferred for the rest).

  • A first run has nothing valid committed, so the working tree still runs.
  • Every new step is inside a try/catch that falls back as before.
  • Only D and G change.

Non-blocking nit: the start note says "started with the checkout's" in every fallback, so an operator can't tell whether HEAD or the working tree ran. Naming which one ran would help.

3. Wording and the PR body (inferred, by reading)

The corrections are true (see the table above).

Small inaccuracies, none of them blocking:

4. Every path into trainMerge (measured by grep and by a probe)

Caller Stop re-checked right before?
mergeTested (batched review, Discord batched, batched auto), merge-flow.ts:594 yes, new
mergeApproved (Discord, not batched), merge-flow.ts:658 yes, already on main
mergeOrWait auto, not batched, merge-flow.ts:557-559, after combinedGate ("can take minutes", per its own comment) emergency only, not the auto-merge stop

Probe on the head build (the repo's harness, with combinedGate wrapped so that it writes auto-merge-stopped.json and then returns merge): stop raised during combinedGate=true merged=true merge.done=1. The control run, with no stop raised, also merged.

Not a regression: main has the same code (origin/main:merge-flow.ts:615-618, inferred). It is still the same class this round fixed, and principle 2 decides it. trainMerge calls itself "the merge train's one door to mergeOurs", so a check there covers every caller at once (principle 4).

Safe to add (inferred): mergeDecision already refuses every auto decision while the stop is in force (merge-policy.ts:240), and both approval paths refuse it too. So a check inside trainMerge changes nothing except the race.

5. Rule 10: the direct-land.test.ts fixture (measured, then judged)

The change is safe, with no assertion changed (measured):

  • The diff is 3 added lines (a comment and git commit -am).
  • I ran the old fixture against the head's code: 4 of 8 fail. That is the same as CI run 39217174535.
  • With the new fixture, all 8 pass.

It models a real instance better (inferred):

  • A real instance's config arrives on main through the merged setup PR, so it is committed.
  • The old fixture had no origin at all and relied on an uncommitted edit.

What existing installs see (judged): this is a function change, not a principle-3 reopening. An existing install changes only in cases D and G: main has no config (or an invalid one), something valid is committed at the checkout's HEAD, and the working tree has uncommitted edits. Such an install now starts on the committed config. Why I judge it this way (inferred):

The upgrade note is there: CHANGELOG "Upgrading (from v0.5.2)" says: "the checkout's committed config runs, not a hand-edited working copy". The PR body discloses the fixture change. Suggestion, not blocking: add "commit an edit you want to run".

Protection (measured)

  • Paths: src/** (engine) and the self-modification parts (review.ts, decide.ts, dashboard-auth.ts, merge-flow.ts). This round adds src/os/index.ts, which is engine.
  • Marking: the title has [PROTECTED: owner merges], the body has **Risk class:** protected, the PR has the needs-owner label, and it has real "What this can never do" and "Compromised component" sections.
  • The "Coordinator user" bullet: it now says that user can set the key today. That is true (add-project.sh:222).

Other checks

  • DCO: all 4 commits signed off (measured).
  • Attribution: no Co-Authored-By or "Generated with" in the commits or the PR body (measured, grep).
  • Hygiene: no new dependencies and no .claude/ change. Closes #715 is present.
  • Adversarial pass: present and updated for round 2. Its steering item was "inferred"; I measured it above.
  • Rule A: the new tests run inside the isolate, and the fake id goes into its tmpdir (inferred from the code).

CI on ce43c049 (measured, gh pr checks 805)

Everything required passes: both test matrices on Node 22 and 24, all 8 shards, dco, denylist, push-gate, scan, scope, new-deps, agentshield and desktop. The real-systemd jobs pass too: add-project, egress firewall, test-run sandbox, the wizard end to end and the sign-in probe. Both bootstrap jobs were skipped, as usual.

merge-tree (measured, git merge-tree --write-tree)

Required changes

  1. Merge origin/main (178c44e6) and resolve the merge-flow.ts conflict. Keep both conditions: batching && !this.emergencyInForce() && !this.autoMergeStopped() && (await this.ownerApproved(w, pr)) && !this.triesCapped(...). Re-run pr-batch-coordinator, merge-idempotent and decision-gates on the merged head and report CI on it.
  2. Fix the class of the stop re-check: put it in trainMerge itself. Add if (this.emergencyInForce() || this.autoMergeStopped()) return null; immediately before mergeOurs, after the backlog.pullRequest await. Keep the existing caller checks. This covers the autonomous non-batched path (measured above: a stop raised during combinedGate still merges), and it closes the remaining pullRequest window on the two approval paths. Add a test that fails without it: a stop written during combinedGate on the non-batched auto path holds the merge. Then "What this can never do" Run a coordinator per instance, with only that instance's logins #3 and separate-users.md's "While it's stopped, every PR waits for a person" are true for every path.

Not blocking

New trade-offs for the owner

None. Item 5 is a function change the issue asked for, with an upgrade note.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>

# Conflicts:
#	src/coordinator/merge-flow.ts
…l; start note names its config (#715)

Review round 2 measured a stop raised while the combined gate runs on the autonomous, non-batched path still
merging: only mergeTested and mergeApproved re-checked it. trainMerge is the one door to mergeOurs, so the emergency
stop and the auto-merge stop are now checked there, immediately before the merge call, after its own GitHub read.
That covers every path into it (the autonomous merge, mergeTested, mergeApproved). The earlier checks stay.

Also: startConfig's note says which config ran (the checkout's committed config at HEAD, or its working tree); two
test messages and design.md's Decisions line no longer call the policy file root's, and name the local site's log
owner for `decide`.

Test (fails without): test/decision-gates.test.ts "auto-merge, not batched: a stop raised while the combined gate
runs holds the merge".

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

Round 3: changes for the round-2 verdict

Head f4853334. Two signed-off commits on top of ce43c049, with no other trailers:

  1. Merge with Merge: idempotent across a crash: merge.attempted before the API call, then reconciled, so the engine never reverts its own good merge (#733) [PROTECTED: owner merges] #793 (measured). The one conflict was merge-flow.ts's batched-review line. The resolution keeps both conditions: !this.autoMergeStopped() and Merge: idempotent across a crash: merge.attempted before the API call, then reconciled, so the engine never reverts its own good merge (#733) [PROTECTED: owner merges] #793's triesCapped (merge-flow.ts:574). pr-batch-coordinator, merge-idempotent and decision-gates all pass on the result, along with live-config and direct-land: 64/64.
  2. The stops at the one door (measured).
    • trainMerge now checks emergencyInForce() || autoMergeStopped() immediately before mergeOurs (merge-flow.ts:452). That is after its own pullRequest read, so the GitHub round trip the reviewer noted is covered too.
    • The existing checks stay (rule 10): the batched review :574, mergeTested :659, mergeApproved :724, and the auto path's emergency check.
    • Every call into mergeOurs (measured by grep):
      • The only caller of backlog.mergePr outside the backlogs is mergeOurs (:393).
      • The only caller of mergeOurs is trainMerge (:455), right after :452.
      • The callers of trainMerge:
        • the autonomous non-batched merge (:624, after combinedGate);
        • mergeTested (:660; batched auto, batched review, and Discord's batched approval);
        • mergeApproved (:725; Discord, not batched).
      • All three pass through :452.
    • New test: decision-gates "auto-merge, not batched: a stop raised while the combined gate runs holds the merge". It wraps combinedGate so that it writes the stop file and then returns merge. Its control, with no stop raised, still merges.
  3. Nits.
    • The two test messages no longer say "root".
    • design.md's Decisions line and "What this can never do" Add instances: a home per project with its own policy and credential references #2 now name the local site's log owner.
    • The body cites :574 instead of :507.
    • The start note now says which config ran: "started with the checkout's committed config (HEAD)", or "…working tree (no valid config committed at HEAD)". The two config tests assert the HEAD form.

Fail-without on ce43c04 (measured): I checked test/decision-gates.test.ts out over ce43c04 in a scratch worktree, built and ran it. Result: tests 14, pass 11, fail 3.

  • auto-merge, not batched: …: AssertionError: not merged: the stop came in before the merge call.
  • The two config tests fail only on the note's new wording. The old note was no .worklane/ at origin/main: started with the checkout's.

Checks (measured): typecheck passes for src and web. check:denylist reports "1295 files clean". check-dco origin/main HEAD reports all signed off. merge-tree with origin/main 178c44e6 is clean.

CI on f485333: every check passes (measured, gh pr checks 805). That covers:

  • both test matrices and all 8 shards;
  • dco, denylist, push-gate, scan, scope, new-deps, agentshield and desktop;
  • the real-systemd jobs (wizard e2e, add-project, egress firewall, test-run sandbox, the Claude sign-in probe).

Bootstrap was skipped as usual. The PR body's Adversarial pass has the round-3 entries.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: PASS

Final check of head f4853334 (round 3), judged by the decision guide. Both round-2 required changes are met, and the four non-blocking nits are fixed. Nothing earlier regressed. CI is green on the head, and the head merges cleanly with current origin/main.

How I ran things: scratch worktrees at f4853334 and ce43c049, each with a scratch HOME, WORKLANE_STATE_DIR, WORKLANE_HELD_OUT_DIR and TMPDIR, and a fake claude first on PATH. I didn't run the mcp-broker tests or the escape fixture. The probes are in the scratchpad under work/eval-805r3/.

Round-2 required changes

# Required change Result Evidence
1 Merge origin/main (#793) and keep both conditions on the batched-review line met (measured) git show --remerge-diff 85e894cd: the only conflict was merge-flow.ts, and the resolution is batching && !this.emergencyInForce() && !this.autoMergeStopped() && (await this.ownerApproved(w, pr)) && !this.triesCapped(...) (merge-flow.ts:574). The tests and #793's cap probe pass on the head (below).
2 Check the stops in trainMerge, right before mergeOurs, with a test that fails without it met (measured) merge-flow.ts:452 checks emergencyInForce() || autoMergeStopped(), after the pullRequest read and the behind deferral, then calls mergeOurs at :455. Fail-without and probes are below.

1. The merge with #793 (measured)

2. Stops at the one door

Fail-without (measured). I checked test/decision-gates.test.ts out from f4853334 over ce43c049, built and ran it. tests 14, pass 11, fail 3, which matches the builder:

  • auto-merge, not batched: a stop raised while the combined gate runs holds the merge… fails with error: 'not merged: the stop came in before the merge call'.
  • The two config tests fail only on the note's new wording (actual "no .worklane/ at origin/main: started with the checkout's").

On the head all 14 pass.

My round-2 probe, rerun (measured). In stop-probe.mjs, a stop is raised during combinedGate on the non-batched auto path.

  • Head: stop raised during combinedGate=true merged=false merge.done=0. The control, with no stop, merges.
  • Round 2 (ce43c049): merged=true.

A new probe for the window round 2 left open (measured). door-probe.mjs raises the stop during trainMerge's own backlog.pullRequest read, which it recognises by trainMerge in the call stack.

Path Stop Head f4853334 ce43c049
auto, not batched auto-merge stop held (merged=false, merge.attempted=0) merged
auto, not batched emergency STOP held merged
Discord mergeApproved, not batched auto-merge stop held merged
Discord mergeApproved, not batched emergency STOP held merged
both none (control) merged merged
  • The builder labelled the mergeApproved path "inferred"; it is now measured.
  • merge.attempted stays at 0 when a merge is held, so Merge: idempotent across a crash (record the intent before the API call, then reconcile), so the engine never reverts its own good merge #733's reconcile sees no open attempt.
  • Recovery (measured).
    • After resumeAll or clearAutoMergeStop, the approved path merges.
    • The emergency stop on the auto path also merges once resumed.
    • When the auto path is held by the auto-merge stop, the next pass re-decides it as auto:false, and it waits for the owner after the stop is cleared. Base and head do exactly the same when a stop is in force before the PR goes green (resume-probe.mjs: [early] … merged=false on both).
    • So this matches existing behaviour and fails closed (principle 1). It doesn't reopen anything.

Every path to the merge API (measured, grep -rn "mergeOurs\|mergePr" src):

  • backlog.mergePr is called only in mergeOurs (merge-flow.ts:389).
  • mergeOurs is called only from trainMerge (:455), right after :452.
  • trainMerge is called from :624 (mergeOrWait, auto), :660 (mergeTested) and :725 (mergeApproved).
  • So every path passes through :452.

Is there an await between the check and the merge call?

  • In the coordinator: none (measured by reading). After :452, mergeOurs runs synchronously up to await this.d.backlog.mergePr(...). log.read, neverAutoMerge, mergeMessage and emit('merge.attempted') are all synchronous (base.ts:303).
  • Known residual, inside the backlog layer (inferred, by reading backlog/github.ts:44-66). mergePr calls req. Before the PUT is sent, req awaits this.token() and budget.take(), which waits out a rate-limit pause for up to DEFAULT_MAX_WAIT_MS = 10 s (github/budget.ts:27). A stop raised in that window, or while the PUT is in flight, can't hold that merge.

3. Rule 10: the two config tests (measured, by diff)

  • Invalid config on main: /is invalid, started with the checkout's/ became /is invalid, started with the checkout's committed config \(HEAD\): /. This is stricter: it now also asserts which config ran and that the error detail follows.
  • No config on main: /no \S+ at origin\/main: started with the checkout's/ became /^no \S+ at origin\/main, started with the checkout's committed config \(HEAD\)$/. This is stricter: it is anchored and names HEAD.
  • The required_checks assertions are unchanged. The two other edits are assertion messages only ("root" became "policy.yaml").
  • Nothing else changed: no test was removed, and no other assertion changed in f4853334. The merge 85e894cd resolved no test files (remerge-diff).
  • The new note names which config ran (measured, start-probe.mjs on the head):
    • Cases D and G say …started with the checkout's committed config (HEAD).
    • Cases A, B, C and E say …started with the checkout's working tree (no valid config committed at HEAD).
    • The note's only consumer is console.error (service.ts:225,318).
    • Small gap, not blocking: no test asserts the working-tree form of the note.

4. Earlier rounds (measured)

  • decide uid: cli.ts:1084 uses currentUid(), which is process.geteuid() (os/index.ts:297-298). Rerunning my round-2 decide-probe.mjs on the head gives the same results as round 2:
    • a fake id printing 4242 on PATH: recorded;
    • every symlink, root-owned or missing-log steer: refused;
    • --instance x with USER=wl-x: refused.
  • Config fallback order: start-probe.mjs cases A-G pick the same config as in round 2: the working tree for A, B, C and E, committed HEAD for D and G, and main for F.
  • Money paths from main:
    • base.ts:276-277 reads refs/remotes/<remote>/<branch>. The only other loadMoneyPaths caller is decision-detail.ts:170 (mainRef). Nothing else reads money_path_source.
    • The decision-gates money-path test passes.
  • Rule 10 on the earlier checks: the auto path's emergency check (:557, :622), the batched review (:574), mergeTested (:659) and mergeApproved (:712-724) are all still in place.

Protection (measured)

  • What this round touches: src/coordinator/merge-flow.ts (self-modification and merge policy), src/config/live.ts (engine), docs/design.md (engine-guards) and a test. That adds no new protected class.
  • Marking: the title has [PROTECTED: owner merges], the body has **Risk class:** protected, the PR has the needs-owner label, and the body has real "What this can never do" and "Compromised component" sections.
  • Principle 3: this round only tightens. It adds a stop check, and the only other changes are a note's wording and test messages.

Other checks (measured)

  • DCO: scripts/check-dco.mjs origin/main f4853334 reports "all commits signed off".
  • Attribution: none in the PR's commits, the PR body or the round-3 comment (grep, 0 hits).
  • Hygiene:
    • npm run typecheck is clean for src and web.
    • check:denylist reports "1295 files clean".
    • There are no new dependencies and no .claude/ change, and Closes #715 is present.
  • Adversarial pass: present, with the round-3 entries (the door, the grep of every path, the note).
  • Rule A: the new test runs inside the isolate and writes the stop file into the fixture's temporary state dir (inferred from the code; my runs used scratch dirs).

CI on f4853334 (measured)

gh pr checks 805: every check passes, and runs 39224707846 and 39224707790 report headSha f485333….

  • both test matrices on Node 22 and 24, all 8 shards;
  • dco, denylist, push-gate, scan, scope, new-deps, agentshield and desktop;
  • the real-systemd jobs: add-project, egress firewall, test-run sandbox, the wizard end to end and the sign-in probe.

Both bootstrap jobs were skipped, as usual.

merge-tree (measured)

git merge-tree --write-tree origin/main f4853334, with origin/main = 178c44e6: clean (tree e7dfe527). origin/main is already an ancestor of the head.

Not blocking

  • The PR body overclaims slightly. "What this can never do" Run a coordinator per instance, with only that instance's logins #3 says the merge holds "including a stop raised while the merge is under way". It holds for anything before backlog.mergePr. A stop raised during mergePr's own token or budget wait (at most about 10 s) or while the PUT is in flight can't hold it. Suggest adding that to "Left open" as a known residual (principle 5; a body edit only).
  • Optional follow-up: pass a pre-send guard into mergePr so the stop is re-read after budget.take(), which would narrow the window to the PUT itself.
  • No test asserts the working-tree form of the start note.

New trade-offs for the owner

None. This round only tightens, by principles 1, 2 and 4. The PR is protected, so the owner merges.

@jayden1711

Copy link
Copy Markdown
Owner Author

Round-3 final check (6263769924) non-blocking item applied by the orchestrator, body only: "What this can never do" #3 is narrowed to the check immediately before the merge call, and the backlog's token/rate-limit wait and in-flight request are listed under Left open. No code change.

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>

# Conflicts:
#	CHANGELOG.md
@jayden1711

Copy link
Copy Markdown
Owner Author

Merge of main 128453a: CHANGELOG only

Head 57312cae is a signed-off merge of origin/main 128453a0, which brings in #809's v0.5.3 layout and #802's notes. The only conflict was CHANGELOG.md. I took main's file and placed #805's bullets at their own anchors:

Checks (measured):

  • scripts/lib/changelog.mjs run in memory (withSection with a regenerated v0.5.3 section): 95 hand-written v0.5.3 lines before and after, 0 dropped. Every older section is byte-for-byte unchanged. One ### Guardrail changes: what each one does heading remains.
  • The changelog, release and release-tree tests: 26/26 pass.
  • Typecheck (src and web) passes, and check:denylist reports "1306 files clean". DCO: all signed off.
  • The code is unchanged since f485333: git diff f4853334 HEAD -- src test scripts has the same git patch-id --stable (e80446bd…) as main's own git diff 178c44e6 128453a0 -- src test scripts, at 62 files, +5754/−314 each.

…quires (#715)

Merging main brought #772's lint, which allows os/index.js only as a static named import; decide (#715) loaded
currentUid, currentUser, fileAccess and isRoot with a dynamic import(). It was dynamic only to follow the lazy imports
of the command modules around it: no cycle, and cli.ts already imports os/index.js statically, so the module was
loaded at startup either way. Behaviour is unchanged (--help, status and decide give the same output before and after).

Test (fails without): test/slot-path.test.ts "src/: only os/index.ts makes a slot path or opens one ..." fails on
57312ca with "src/cli.ts:1078: import() of slot-file.js or os/index.js: import its functions by name".

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
@jayden1711

Copy link
Copy Markdown
Owner Author

The merged trees collided with #772's slot-path lint; the fix is a static import

After the main merge (57312cae), CI shard 4/4 failed on both Node versions (measured, run 39279206303).

Fix (bf0a1bba): I deleted the dynamic import and added the four names to cli.ts's existing static import … from './os/index.js' line.

Why it was dynamic (measured from history): git log -S "import('./os/index.js')" -- src/cli.ts shows it came in with this PR's own 875419a. It only copied the lazy await import() style of the command modules around it (decide.js, setup/app-manifest.js). There was no import cycle and no startup reason. cli.ts already imports os/index.js statically on three lines, so the module loaded at startup either way.

Behaviour unchanged (measured): I built 57312ca and bf0a1bb side by side and ran the CLI in scratch HOME and state dirs.

Checks (measured):

  • slot-path and decision-gates: 47/47 pass on bf0a1bb.
  • The slot-path test fails on 57312ca with the message above. That is the fail-without.
  • Typecheck (src and web) passes, check:denylist reports "1306 files clean", and DCO is all signed off.

CI on bf0a1bb is running. I'll report it once it finishes.

@jayden1711

Copy link
Copy Markdown
Owner Author

Evaluator verdict: PASS

Final check of head bf0a1bba (round 4), after the round-3 PASS at f4853334. Since that pass the branch has two new commits: a merge of main 128453a0 (CHANGELOG conflict only), and bf0a1bba, which turns decide's dynamic import('./os/index.js') into static named imports. Nothing else changed. CI is green on the head. It merges cleanly with current origin/main, with #787 and with #820. Against #780 it conflicts in CHANGELOG.md only, and that conflict is mechanical.

How I ran things: scratch worktrees at bf0a1bba and 57312cae (the merge, before the import change). Installs went through the owner's npmw.sh wrapper, and builds ran inside each worktree. Every CLI and test run used a scratch HOME, WORKLANE_STATE_DIR, WORKLANE_PROJECT_STATE_DIR, WORKLANE_HELD_OUT_DIR, WORKLANE_CONFIG_DIR, WORKLANE_INSTANCES_DIR and TMPDIR, with a fake claude first on PATH. I ran named test files only.

1. The delta since f485333 is main's plus the cli.ts import (measured)

  • The merge base of f4853334 and 128453a0 is 178c44e6.
  • Per file, 62 of the 63 files under src, test and scripts have the same git patch-id --stable in f4853334..bf0a1bba as in main's own 178c44e6..128453a0. As a whole set without src/cli.ts, both give e80446bd…. Main didn't touch src/cli.ts.
  • The only other diff is in src/cli.ts:
    • the existing static import becomes import { currentUid, currentUser, fileAccess, homeDir, isRoot } from './os/index.js';;
    • the line const { currentUid, currentUser, fileAccess, isRoot } = await import('./os/index.js'); is deleted.
  • Outside those three directories, every file matches main's delta except CHANGELOG.md (section 3).

2. The import change

Behaviour is unchanged (measured). I ran each command on both builds and compared the full output plus the exit code:

Command Result on both builds Same?
--help usage text, exit 0 yes
status --root examples/basic (unrelated command) decisions waiting: none, exit 0 yes
decide usage: decide <decision-id> <option>, exit 2 yes
decide d-x approve (local checkout) no decision d-x, exit 1 yes
decide d-x approve --root examples/basic no decision d-x, exit 1 yes
decide d-x approve --instance example decide: refused: only root or example's coordinator user (wl-example) may answer its decisions from the CLI, not <user>, exit 1 yes

The events log was written under the scratch project state dir only.

No import cycle and no startup change.

  • Measured: before the change, cli.ts already imported ./os/index.js statically on 3 lines, so the module was already loaded at startup. src/os/index.ts imports only node built-ins, brand, network, project-env, guardrails/push-hook, slot-file and ./system-paths, and no module in src imports cli.js. --help ×5 averaged 355 ms before and 336 ms after (noise-level).
  • Inferred: no new module enters the startup graph, so there is no cycle.

Tests (measured):

3. CHANGELOG (measured, in memory with scripts/lib/changelog.mjs, main 128453a0 vs bf0a1bba)

4. CI on bf0a1bb (measured)

gh pr checks 805: every check passes. Runs 39286060787 (ci) and 39286060485 (ci slow) report headSha bf0a1bba…, conclusion success.

  • both test matrices on Node 22 and 24, all 8 shards;
  • dco, denylist, push-gate, scan, scope, new-deps, agentshield and desktop;
  • the real-systemd jobs: add-project, egress firewall, test-run sandbox, the wizard end to end and the sign-in probe.

Both bootstrap jobs were skipped, as usual. installed-engine-config doesn't apply: it is filtered to .worklane/**, and #805 changes nothing there against main.

5. merge-tree (measured, git merge-tree --write-tree, read-only)

Against Result
origin/main = 128453a0 clean (tree 0c4ce910). Main is already an ancestor of the head.
#787 at 20524e06 (moved from 8408283d) clean (tree 0c8c9203)
#820 at a3fc766c clean (tree 13474c23)
#780 at a021bd8f conflict in CHANGELOG.md only; docs/test-runs.md auto-merges

About the #780 conflict:

Other checks (measured)

Required changes

None.

New trade-offs for the owner

None. This round brings in main and makes a behaviour-neutral import change. The PR is protected, so the owner merges.

@jayden1711
jayden1711 merged commit 3cf33a5 into main Oct 19, 2026
26 checks passed
@jayden1711
jayden1711 deleted the issue-715-approval-gate branch October 19, 2026 18:20
jayden1711 added a commit that referenced this pull request Oct 19, 2026
…nto issue-545-slirp-test-user

Conflicts:
- src/project-env.ts RESERVED_NAMES: both lists kept (#780's glibc unsafe-environment names and LIBMOUNT_*,
  LANGUAGE; #787's SHELLOPTS, BASHOPTS, GLOBIGNORE, EXECIGNORE, BASH_*).
- CHANGELOG.md, at the coordinator's anchors:
  - #780's Upgrading bullets go before "The dashboard never takes a Claude credential (#749, #751)". The
    bash-names bullet now says #713 refuses SHELLOPTS, BASHOPTS and BASH_* at the config check.
  - #780's test-user.sh install step is folded into #809's egress bullet: the installs with #809's caveats,
    then test-user.sh check per instance, then the egress check.
  - #780's "No new sudoers rules" line is dropped, since #809's is fuller.
  - #780's guardrail bullet goes before "The host egress rule covers the test-run users by default".

Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
jayden1711 added a commit that referenced this pull request Oct 19, 2026
test: raise the package budget to main's size + 2% after #805 and #787 [PROTECTED: owner merges]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approvals: the same role gate on every surface (GitHub comment, dashboard, CLI), and money paths read from main

1 participant