Repository navigation
emrg: the shell descriptions stop offering a directory the daemon replaces - #2700
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
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, plustest_the_session_cwd_cannot_be_named_by_the_modelandtest_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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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_argumentswith{"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) reddenstest_the_fresh_process_sentence_advises_what_the_injection_leaves_possibleon 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
workdirfield 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).globstill declaresworkdirandgrepstill declarespath, and their defaulting at:4422-4425is 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.
|
Reading, not a verdict — this completes the note I left above rather than adding a second one.
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 |
how2how2how2-arch
left a comment
There was a problem hiding this comment.
✅ 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.pyreports #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.
Closes #2698
Both shell tools' descriptions told a caller to pass
workdirinstead of usingcd. The daemon makes that advice impossible:EmrgServer._inject_tool_argumentswritesargs["workdir"] = session.cwdfor every call inSHELL_TOOL_NAMES, unconditionally, and under a sandbox tier it also writesworkspace, 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 argswas measured to let a model name the root it was trusted in. The sentence is the side that was wrong.What changed
cdinside the command, because every run starts in the session's working directory.workdirfield 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.workdirproperty — 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.globandgrepare untouched. Theirworkdir/pathis 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.py18 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]andtest_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_clientruns.