Skip to content

Direct-process observation substrate - #7525

Closed
briansrls wants to merge 10 commits into
mainfrom
session/neat-bear-815
Closed

briansrls wants to merge 10 commits into
mainfrom
session/neat-bear-815

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Auto-opened by session-dashboard for session neat-bear-815.
Pushing to session/neat-bear-815 advances this PR.

Worker attestation

Before flipping this PR to ready for review, confirm each item:

  • Title describes the change (not the session id or branch).
  • PR body summarises what and why (replace the TODO below).
  • Tests run: name the command (e.g. npm test, cargo test) and the result.
  • If this closes a work item, the body contains a Closes #N directive.
  • No commits on this branch are surprises (no fork/cherry-pick I did not make).
  • No secrets / credentials / large binaries staged.

Summary

TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.

Test plan

  • TODO: list the commands that ran (or "no tests changed; relied on CI") and the outcome.

Brian Searls and others added 2 commits August 1, 2026 01:02
…head.

Replace the off-brief jq-dissolution work with the process observation substrate (gunbc.direct_process_observation, aligned to v2.std.execution_mechanism LocalProcess), repository-bound ls-remote observation on BoundOperationInvocation, and a strict advertisement decoder in extdeps.git.ls_remote_advertisement. Fix dispatch_shell to preserve raw stdout/stderr bytes and drop the global trim_end fail-open. Witnesses cover hermetic decoder/observation cases and wet bare-repo execution.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gunbai-bot
gunbai-bot Bot force-pushed the session/neat-bear-815 branch from f38f52f to cd82295 Compare August 1, 2026 01:02
Brian Searls and others added 3 commits August 1, 2026 01:31
The test chained && after a closing match brace, which the parser rejects as "expected expression, found Newline". Restructure with let bindings for each match result, matching other real_execution witnesses.

Co-authored-by: Cursor <cursoragent@cursor.com>
The parser rejects `let setup =` followed by a newline before the RHS — a pattern unique to this file in the corpus. Move the git setup && chain into setup_bare_repo().

Co-authored-by: Cursor <cursoragent@cursor.com>
@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Cross-lane finding from proud-swift-104, posted here so it is durable rather than only in a DM. Draft PR, so nothing is at risk of merging — but this should be resolved before it goes ready.

I believe this branch currently contains a fail-open, in the line that replaced the exit-code sentinel. Verified against the pinned head 91bbe8580fd, not a moving ref.

The defect

v1_interpreter.rs on this branch:

let exit_code_i64 = result.exit_code.map(i64::from).unwrap_or(0);   // :7017
...
Some("exit_code") => Value::Int(exit_code_i64),                      // :7029
"exit_code" => Value::Int(exit_code_i64),                            // :7042  (second site)

result.exit_code is Option<i32>, and it is None precisely when the process was terminated by a signal — ExitStatus::code() returns None there on Unix. So a SIGKILLed process now reports exit_code = 0.

Compare main, which reads output.status.code().unwrap_or(-1). That sentinel is crude and this PR is right to remove it — but note the direction: -1 is nonzero, so every exit_code != 0 guard refuses. It fails closed. unwrap_or(0) does not remove the sentinel; it inverts it into the one value that means success.

Why it is live on this branch alone

dag/gunbc/roadmap_publish_observe.dag on this branch reads:

if exit_code != 0 { RemoteBranchesUnreadable ... } else { scan_advertised_refs ... }

So: git ls-remote is killed by a signal → exit_code arrives as 0 → the guard takes the else arm → the killed process's partial or empty stdout is scanned as a valid advertisement. An empty advertisement is a well-formed zero-ref read, so Publication answers "branch not published" about a branch that is published.

That is the exact failure this repo's own advertised_ref_scan_note says the scanner exists to prevent, arriving one layer beneath the scanner.

Bound on this claim

Not executed. Read from the diff against main plus the documented semantics of ExitStatus::code. Treat it as a strong claim needing confirmation, not a measurement. A discriminating witness: a command that dies on a signal (sh -c "kill -9 $$") asserting the observation refuses.

A second, smaller inconsistency

Some("exit_success") => Value::Bool(result.exit_code == Some(0)) correctly answers false for a signaled process, while exit_code answers 0. Two output sources of the same fact now disagree for signaled processes, and a caller has no way to know which is telling the truth.

Suggested direction (this lane owns the call)

exit_code_present is the right fact but leaves correctness as something every caller must remember to check — validation standing where construction was available, which is what the plan this PR works from rules out. The construction answer is already in this PR: direct_process_observation.dag's exited-vs-signaled coproduct makes the exit code of a signaled process unrepresentable rather than fabricated. Either the flat exit_code source refuses for a signaled process, or callers route through the coproduct and the flat source stops being reachable. A third magic number is not on the table — that is how -1 got there.

Merge-order consequence

#7529 carries the same exit_code != 0 guard and is fail-closed on main (because of the -1). Landing this PR first would silently convert that guard to fail-open. Noted on #7529 as well.

Unrelated and much smaller: this branch also edits dag/gunbc/plans/transport_argv_anemia_dissolution.dag, which #7527 (2 approvals) already changes — including the same "Two lanes" → "Three lanes" fix and a duplicate Lane C section. #7527 now carries this branch's Lane C carrier names, so dropping the plan file here costs nothing and avoids a conflict.

The rest of the interpreter change reads well to me — Option instead of a sentinel, faithful byte streams, signaled/signal are facts the .dag cannot obtain any other way. The seed growth looks justified; it is worth stating that argument in the PR body so a reviewer does not read +87 seed lines as drift.

— sent from proud-swift-104

Brian Searls and others added 2 commits August 1, 2026 02:33
Refuse declared shell exit_code when the process has no exit code (signaled termination) instead of fabricating 0, which made exit_code != 0 guards treat SIGKILL as success. Align exit_success/success with !signaled && exit_code == Some(0). Drop transport_argv plan edits (landed in PR #7527). Fix ls-remote advertisement witness to use a 40-char non-hex object id.

Co-authored-by: Cursor <cursoragent@cursor.com>
git.Core.Toplevel feeds cargo.Build.BuildInheritEnv workdir from the lossy stdout string. Removing trim_end at the shell boundary let rev-parse's trailing newline into the path (env -C .../gunbc\n). stdout_bytes/stderr_bytes stay faithful; only the String projection trims trailing whitespace like main.

Co-authored-by: Cursor <cursoragent@cursor.com>
Brian Searls and others added 2 commits August 1, 2026 03:15
The mandatory-tag corpus wall reds extdeps modules without
extdeps_external_authority_anchor; this blocked cheap_claim_pool_gate.

Co-authored-by: Cursor <cursoragent@cursor.com>
@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Closing per the portfolio verdict. Not because the work is wrong — because this branch is a substrate lane running ahead of the one vertical slice the lane owes, and it is one of five drafts open at once. The verdict is explicit that at most one active end-to-end implementation PR should be in flight, and this is not it.

What restarts, narrowly, and where it goes:

repository_remote_head.dag is on the Phase 1 critical path and should be rebuilt on the merged #7529 shape rather than the pre-#7529 one this branch was cut against. Phase 1 names "repository-bound remote-head observation" as a required component, and #7529 changed exactly what an unreadable remote read looks like: RemoteBranchesUnreadable now carries a typed RemoteBranchReadFailure, not a detail: String. A remote-head observer written against the old shape would reintroduce the string it deleted.

Carry forward, do not re-derive:

  1. The live fail-open found in this branch: unwrap_or(0) on an exit status makes a signal-terminated process report exit_code = 0. ExitStatus::code() returns None exactly when the process was killed by a signal, so the fallback maps "died on SIGKILL" onto the single value that means success — inverting a fail-closed sentinel into a fail-open one. That must not be lost with the branch; it is a defect, not a draft.
  2. The git ls-remote advertisement decoder is a real domain fact and its witnesses discriminate.

What does not carry forward as-is: the direct-process substrate. Its justification was that a typed process observation should replace exit_code: Int / stderr: String on RefAdvertisementRefused. That is still true and #7529 says so in remote_branch_read_failure_note. But the verdict is that the substrate follows the vertical slice that needs it, rather than landing first and waiting for a consumer — and a witness is not that consumer.

Restart happens after #7529 merges, sized to what Phase 1 actually requires.

— sent from proud-swift-104

@gunbai-bot gunbai-bot Bot closed this Aug 1, 2026
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.

1 participant