Repository navigation
SHELL-DAG: migrate six retained_foreign call sites to typed operations - #8621
Merged
Merged
Conversation
Dissolves six confirmed retained_foreign (shell-string-concat) call sites in favor of typed shell/extdeps operations, per §3/§5: construction over string-building, single authority reused where it exists. Sites migrated (all in dag/gunbc/package_delivery.dag unless noted): 1. observe_protocol_schema_identity — codex app-server generate-json-schema invocation -> new codex_app_server.Cli.GenerateJsonSchema (added to the existing extdeps.llm.codex_app_server module, which already cites this executable/protocol). 2. observe_protocol_schema_identity — `find ... | sort` listing -> new shell.Find.FilesByNameSorted (extdeps/shell.dag), following the existing FilesAndSymlinksWithMode positional-parameter precedent so no path/glob can escape a quote. 3. finish_bisect_generate_protocol_schema_dir — duplicate of #1, same new op. 4. relocate_build_root_to_unique_publishing — `rmdir && mv -T` -> shell.Remove.EmptyDirectory + shell.Move.NoReplaceDirectory (both new operations on the existing shell.Remove/shell.Move services). 5. rename_publishing_noreplace — `mv -T` -> shell.Move.NoReplaceDirectory. 6. dag/tools/interpreter_dispatch_bijection_real_roster_transport.dag, real_roster_cargo_run — multi-line bash (set -euo pipefail; cd; export; two `cargo test --ignored --exact` invocations) -> new cargo.Build.TestWithCwdEnv (extdeps/rust/cargo_build.dag), composing `env -C <workdir> <VAR=val...> <cargo> test <args>` as one argv vector, called twice sequentially from real_roster_cargo_test_run with explicit caller-side short-circuit (cargo_outcome_holds) preserving the fail-fast semantics without shell chaining. Architectural calls made along the way (documented per task brief): - Used a narrow, domain-specific op (codex_app_server.Cli.GenerateJsonSchema) instead of shell.Exec.RunArgv + ProcessArgvExpansion/CliSurface, which has zero production consumers today and is disproportionate machinery for a one-off fixed 4-token argv. - shell.Remove.EmptyDirectory uses `rmdir` (refuses if non-empty) rather than `rm -rf`, preserving the original script's safety property. - relocate_build_root_to_unique_publishing computes a plain Bool in each branch rather than unifying two different services' anonymous output records across an if/else, to avoid a speculative cross-type merge. Also fixes two DESIGN §4c violations surfaced while landing the new operations: explanatory `//` comments were nested inside a `service { }` body (module-item grain only) — relocated into top-level `data ..._note` declarations in shell.dag and cargo_build.dag, matching the file's existing convention. And fixes an accidental interpolation hazard: the new cwd/env note's prose contained a literal `{workdir}`, which the compiler parsed as an unbound interpolation reference — reworded to plain prose. Verification: entry-scoped `gunbc compile` on both touched leaf files (package_delivery.dag, interpreter_dispatch_bijection_real_roster_transport.dag) is 0 blocking errors on both (395 / 14 pre-existing-pattern advisories, where-refinement-unenforced on non-literal FilePath/NonEmptyStr casts, unrelated to this change). gunbc.retained_shell_script's retained_foreign carries no fixed caller-count assertion, and none of these six sites appear in the host_language_transport_script wall_residue_live_test.dag roster, so no lens update is required. The interpreter_dispatch_bijection_real_roster witness (real git clone + cold cargo build, ~692s wall) was not executed in this session — its own test file documents it as "a cadence job, not a session job" requiring a runner with ~11 GiB and a local cargo; the hermetic cargo_execution_placement_witness_test.dag controls (which cover the placement-decision logic this site also uses) pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WaiMx97tLCspkMg9rDkfjS
…ation review on #8621 (eager-crane-282): the mktemp placeholder being non-empty (an exclusivity violation -- something else wrote into a name this call believed was exclusively its own) and an ordinary move refusal (destination exists, cross-device, permissions) against a confirmed-empty placeholder were collapsed into one Bool at the binding, even though the new typed operations (shell.Remove.EmptyDirectory, shell.Move.NoReplaceDirectory) had just created the distinction. Introduces UniquePublishingRelocateOutcome (Ready | PlaceholderNotEmpty | Refused) so the caller sees the two failure states separately and reports a distinct cause string for each. The not-empty arm no longer runs the recursive-force cleanup, since rm -rf on a placeholder this call does not own would destroy whatever the other writer put there; cleanup stays on the move-refusal arm where the placeholder is confirmed empty and ours. rename_publishing_noreplace is left untouched (pre-existing, out of scope per the review).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Dissolves six confirmed
retained_foreign(shell-string-concat) call sites onto typed shell/extdeps operations (dashboard node adhoc-a84c2852-5b9), per §3/§5: construction over string-building, reusing existing single authorities rather than forking.Sites migrated
dag/gunbc/package_delivery.dagobserve_protocol_schema_identity— codexapp-server generate-json-schema -oinvocation → newcodex_app_server.Cli.GenerateJsonSchema(added toextdeps.llm.codex_app_server, which already cites this executable/protocol — not a new product surface).find ... -type f -name '*.json' | sortlisting → newshell.Find.FilesByNameSorted(extdeps/shell.dag), following the existingFilesAndSymlinksWithModepositional-parameter precedent so no path/glob can escape a quote.finish_bisect_generate_protocol_schema_dir— duplicate of Add SVG viz, test helpers, and makegen scaffold #1, same new op.relocate_build_root_to_unique_publishing—rmdir '<staged>' && mv -T '<build_root>' '<staged>'→shell.Remove.EmptyDirectory+shell.Move.NoReplaceDirectory(two new operations on the existingshell.Remove/shell.Moveservices).rename_publishing_noreplace—mv -T→shell.Move.NoReplaceDirectory.dag/tools/interpreter_dispatch_bijection_real_roster_transport.dagreal_roster_cargo_run— multi-line bash (set -euo pipefail; cd; export; twocargo test --ignored --exactinvocations) → newcargo.Build.TestWithCwdEnv(extdeps/rust/cargo_build.dag), composingenv -C <workdir> <VAR=val...> <cargo> test <args>as one argv vector, called twice sequentially fromreal_roster_cargo_test_runwith explicit caller-side short-circuit (cargo_outcome_holds) preserving fail-fast semantics without shell chaining.No sites were left as
retained_foreign— all six are fully migrated.Architectural calls made (documented per task brief)
codex_app_server.Cli.GenerateJsonSchemainstead ofshell.Exec.RunArgv+ProcessArgvExpansion/CliSurface, which has zero production consumers today and is disproportionate machinery for a one-off fixed 4-token argv — matches the narrow-fixed-argv-per-tool pattern used everywhere else.shell.Remove.EmptyDirectoryusesrmdir(refuses when non-empty), preserving the original script's safety property rather than silently loosening it.Boolin each branch rather than unifying two different services' anonymous{success: Bool}output records across anif/else, to avoid a speculative cross-type merge.env -C {workdir}argv-splice precedent already incargo_build.dag(BuildInheritEnv,BuildManifest), adding aList<String>env-var-words parameter following theextra_argsprecedent — no new grammar/transport machinery needed.Follow-up: state-space conflation in site 4 (review on this PR, 69f9364)
relocate_build_root_to_unique_publishingoriginally collapsed two distinct typed outcomes —shell.Remove.EmptyDirectoryfailing (placeholder not empty: something else wrote into a name this call believed was exclusively its own) vs.shell.Move.NoReplaceDirectoryfailing (ordinary move refusal against a confirmed-empty, confirmed-ours placeholder) — into oneBoolat the binding. Under that Bool, the not-empty case ranshell.Remove.RecursiveForceon the placeholder, i.e. deleted a directory whose contents this call could not account for. Nobody flagged that as a defect during initial review; it fell out of the decomposition once the two outcomes were split intoUniquePublishingRelocateOutcome { Ready | PlaceholderNotEmpty | Refused }. ThePlaceholderNotEmptyarm now skips the cleanup;RecursiveForceonly runs on the move-refusal arm, where emptiness is established.Caveat on the compile-clean claim below:
compile_clean_diagnostic_is_advisoryadmitsWhereRefinementUnenforcedonto the non-blocking allowlist for any non-literal refined value, andwhererefinements are unchecked at runtime for casts whose kernel isString(which isNonEmptyStr's case) — so "0 blocking errors" is not evidence that any non-literalas NonEmptyStr/as FilePathcast in this diff actually holds. The two newas NonEmptyStrcasts added here are on string literals, not non-literal expressions, so they fall outside that gap; no claim in this PR rests on an unchecked refinement.Incidental fixes
//comments were nested inside aservice { }body (module-item grain only is admitted) — relocated into top-leveldata ..._notedeclarations inshell.dagandcargo_build.dag, matching the files' existing convention.{workdir}, which the compiler parsed as an unbound interpolation reference (undefined variable 'workdir') — reworded to plain prose.NonEmptyStrimport incargo_build.dag(advisory-level, fixed for correctness).Verification
gunbc compileon both touched leaf files is 0 blocking errors on both:dag/gunbc/package_delivery.dag: 395 advisory diagnostics, all the file's pre-existingwhere-refinement unenforcedpattern on non-literalFilePath/NonEmptyStrcasts, unrelated to this change.dag/tools/interpreter_dispatch_bijection_real_roster_transport.dag: 14 advisory diagnostics, same pre-existing pattern plus two on my own newas NonEmptyStrcasts, consistent with the file's existing style.gunbc.retained_shell_script'sretained_foreigncarries no fixed caller-count assertion, and none of these six sites appear inhost_language_transport_script'swall_residue_live_test.dagroster — no lens update required.dag/test/claim/cargo_execution_placement_witness_test.dag(hermetic controls covering the placement-decision logic site 6 also uses) passes — ranw_local_runner_admits_its_observed_pathviagunbc run --claim-run.interpreter_dispatch_bijection_real_rosterwitness (real git clone + cold cargo build, ~692s wall) was not executed in this session — its own test file documents it as "a cadence job, not a session job" requiring a runner with ~11 GiB and a local cargo.gunbc run --dry-runon the unmodifiedreal_roster_cargo_decisionconfirms it hits the expected hermetic-mock wall atshell.Which.Check(pre-existing behavior, unrelated to this change).Test plan