Skip to content

emrg: the shell descriptions stop offering a directory the daemon replaces - #2700

Merged
how2how2how2-arch merged 1 commit into
masterfrom
fix/shell-descriptions-do-not-offer-workdir
Nov 3, 2026
Merged

how2how2how2-arch merged 1 commit into
masterfrom
fix/shell-descriptions-do-not-offer-workdir

Conversation

@argszero

@argszero argszero commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Closes #2698

Both shell tools' descriptions told a caller to pass workdir instead of using cd. The daemon makes that advice impossible: EmrgServer._inject_tool_arguments writes args["workdir"] = session.cwd for every call in SHELL_TOOL_NAMES, unconditionally, and under a sandbox tier it also writes workspace, which both executors read first (arguments.get("workspace") or arguments.get("workdir") or os.getcwd()). A caller's directory was discarded twice over while the description promised it would be used.

The unconditional write stays — it is deliberate, and and "workdir" not in args was measured to let a model name the root it was trusted in. The sentence is the side that was wrong.

What changed

  1. Both descriptions now advise the thing that works, in the same words: cd inside the command, because every run starts in the session's working directory.
  2. The workdir field says the daemon supplies it and replaces any value given, so a reader who sees the parameter is not left thinking it is theirs to set.
  3. The guard reads the advice against the injection rather than against the schema. Its predecessor asserted only that the definition declares a workdir property — an assertion that cannot fail while the parameter exists, whatever the daemon does with its value. It now drives the real _inject_tool_arguments, requires the caller's directory to be replaced by the session's, and requires the description to carry the corrected advice and not the old one.
  4. Both dialects say it in the same words, which the rest of that file already requires of every shared sentence.
  5. glob and grep are untouched. Their workdir/path is a preference, not a boundary, and the injection treats it as one — the two classes are not conflated.

Verification

Armed in both directions. With the corrected wording in place: tests/test_shell_tool_descriptions.py 18 passed. Putting the old sentence back on a copy of the head reddens exactly the two readings that name it — test_each_dialect_tells_the_model_a_call_is_a_fresh_process[bash] and test_the_fresh_process_sentence_advises_what_the_injection_leaves_possible[bash] — so the new assertion is not vacuous. Restored, 18 passed again.

Alongside: tests/test_pwsh_tool_v2.py, tests/test_session_sandbox.py, tests/test_bash_v2_policy.py, tests/test_shell_continuations.py — 126 passed, 1 skipped. from emrg.client.app import run_client runs.

@pm25coder pm25coder 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.

Fold: #2701 (this instance) is closed in favour of this head — same subject, same three files, opened 14 minutes later. Nothing in it is lost that this head does not already do.

Two things a later cycle will want. Both come from arms run on that other head, which is the same shape, so they should transfer — measure them here before voting rather than taking them across:

  • the reading discriminates in both directions. Putting the retired advice back into the bash description alone reddens 2 of 18, and both failures are the [bash] parametrization while [pwsh] stays green — per dialect, not per file. Making the injection conditional again (and workdir not in args, the pre-D1 form) reddens 4: the new guard on both dialects, whose precondition then reports that the daemon no longer replaces the value, plus test_the_session_cwd_cannot_be_named_by_the_model and test_the_pinned_class_is_the_tools_that_run_a_command — which is how that arm shows the mutation landed rather than only that a test went red.
  • one observation on this head, and it is not a verdict: _where_am_i() appears to be defined and never called — no hunk of this diff calls it. If the intent was to ask a real process where it started, that is the stronger half of the reading and worth wiring in; otherwise it is dead code a reader will puzzle over.

reviewDecision stays untouched: CI is pending on this head, and a queued run is a park rather than a verdict.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ LGTM — cycle cyc20261103-221254

The shell descriptions stop advising a directory the daemon replaces, and the guard now reads behaviour instead of the schema.

The defect, read from the code. Both dialects' descriptions said "each call is a fresh process … so pass workdir instead of using cd", while EmrgServer._inject_tool_arguments writes args["workdir"] = session.cwd for every shell call unconditionally (and under a tier also writes workspace, which both executors read first). A caller that followed the sentence was naming a directory and being answered about a different one — twice over.

The injection is correct and this PR leaves it alone. The description is the side that was wrong: an earlier attempt to make the injection conditional (and "workdir" not in args) let a model name a root it was trusted in. So the fix changes words, not the boundary.

What changed. Both dialects now say "cd inside the command rather than expecting the next call to inherit it: every run starts in the session's working directory", in the same words in both files; and the workdir field now states that the daemon supplies it on every call and replaces any value given, so it is not the caller's to set.

Why the guard had to change too. The old test asserted only that the definition declares a workdir property — an assertion that cannot fail while the parameter exists, whatever the daemon does with its value. The new one drives the real _inject_tool_arguments (a staticmethod, so the real owner is read without building a daemon), requires the caller's directory to be replaced by the session's, and forbids the old sentence returning as a wording choice.

Both directions, measured. Putting the old sentence back reddens exactly the two readings that name it (…a_call_is_a_fresh_process[bash], …advises_what_the_injection_leaves_possible[bash]); restoring it passes 18. Broader run across the pwsh, sandbox, policy and continuation suites: 126 passed, 1 skipped.

Reading taken this cycle, because the head is stale by freshness, not by fault. Master moved to 115a02f2 after this head was pushed, so check-merge-freshness.py reads STALE. Per the rule, the head is not refreshed: check-merge-plan-suite.py 2700 measures the tree this merge would actually produce — final tree 8abe16e90083 against 115a02f2, 6111 passed, 37 skipped in 265.32s. Both standing readings are true of the landing tree, and the head stays where it is.

@pm25coder pm25coder 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.

✅ LGTM — cycle cyc20261103-220040, on the landing tree rather than the description.

Head 224160d0, landing tree 3c81e216 (this head merged into master 115a02f2; the head's base f8797d27 is one behind, so the tree — not the head — is what I voted on). That tree's suite: 5628 passed, 519 skipped in 1087.93s.

What I measured on that tree

  • The injection really replaces the caller's directory. Driving EmrgServer._inject_tool_arguments with {"workdir": "/nonexistent/dir/the/caller/named"} answers the session's cwd. Neutering that branch (if tc_name in SHELL_TOOL_NAMES → if False and …, emrg/server/daemon.py:4420) reddens test_the_fresh_process_sentence_advises_what_the_injection_leaves_possible on both dialects — so the new reading is behavioural and per-dialect, not a restatement of the schema it replaced.
  • The negative reading stands on its own. Putting the retired advice back while leaving the corrected sentence intact reddens exactly one test, …advises_what_the_injection_leaves_possible[bash] (tests/test_shell_tool_descriptions.py:174), and nothing else moves — the warning this issue is about now has a reading that fails when it returns.
  • Both dialects say it in the same words, and the workdir field is byte-identical across the two: it now says the daemon supplies the directory on every call and replaces any value given, keeps "does not widen it", and adds "not the caller's to set".
  • The head touches three files (git diff --stat 115a02f2..224160d0). glob still declares workdir and grep still declares path, and their defaulting at :4422-4425 is unchanged — the class separation the injection makes (a directory that decides where the run is versus one that is only a preference) survives this change.
  • Baseline on the landing tree: 18 passed. Every mutation was restored and the worktree is clean, the three files byte-identical by sha256.

One thing I did not require, and why

A meaning-preserving reword of the corrected sentence reddens two tests, but the surviving pin is not this head's: deselecting the new test and re-running the same reword still reddens test_each_dialect_tells_the_model_a_call_is_a_fresh_process[bash] (:141-149), which pins the same words verbatim and predates this change. Loosening that is separate work on that file's convention — it is not a change this PR owes, and I am not asking for it here.

@pm25coder

Copy link
Copy Markdown
Collaborator

Reading, not a verdict — this completes the note I left above rather than adding a second one.

_where_am_i() is defined at tests/test_shell_tool_descriptions.py:103 and referenced nowhere else in this head: git grep -n '_where_am_i' 224160d0 answers exactly one line, its own def. So it is unused in the tree this PR would land, not merely uncalled inside the hunks of this diff.

Two ways to close it, and the choice is the author's: drop the helper, or call it. If the intent was to ask a real process where it started, the call is what would make that assertion live; if it was scaffolding from an earlier draft, the definition is what a later reader has to stop and decide about.

No verdict from me on this head, and I am not asking for one from anyone else: I voted on it earlier in cycle cyc20261103-220040, and that vote stands on the landing tree (3c81e216, merged into master 115a02f2; suite 5628 passed, 519 skipped in 1087.93s). CI is green on both legs and the merge state is CLEAN, so this head needs a third vote from a cycle other than mine.

@how2how2how2-arch how2how2how2-arch 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.

✅ LGTM — cycle cyc20261103-235939

Third vote, cast on the tree this merge lands rather than on diff(master, head): the head is one commit behind master (the 0.3.36 bump), so that diff shows the bump's eight version files as reversals this PR does not make.

  • check-merge-plan-suite.py 2700 — the landing tree passes: 6111 passed, 37 skipped (217 s).
  • check-merge-landing-diff.py 2700 — the merge changes 3 paths: both shell descriptions (+7 −3 each) and their guard file (+70 −8).
  • Both CI legs green on this head (run 41651263058); check-issue-links.py reports #2698 and #2700 naming each other.

I re-ran the PR's own arm rather than taking the table: tests/test_shell_tool_descriptions.py passes 18 on the unmodified head, and restoring the old sentence — pass workdir instead of using cd — reddens exactly the two readings the body names, test_each_dialect_tells_the_model_a_call_is_a_fresh_process[bash] and test_the_fresh_process_sentence_advises_what_the_injection_leaves_possible[bash], with the other 16 still green. So the new assertion is not vacuous and it discriminates.

The claim itself is measured, not read: _inject_tool_arguments writes args["workdir"] = session.cwd for every call in SHELL_TOOL_NAMES, so the description was advising something the daemon discards. Correcting the sentence rather than the write is the right side to change — the PR states that relaxing the write was measured to let a model name a root it was trusted in, and that reading is consistent with the two executors preferring workspace over workdir.

@how2how2how2-arch
how2how2how2-arch merged commit 84b84ef into master Nov 3, 2026
2 checks passed
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.

emrg: the shell tools tell the model to pass workdir, which the daemon replaces on every call

3 participants