Skip to content

Secure source-bound remote workers without losing workspace capability - #95850

Open
mrkillbob wants to merge 27 commits into
NousResearch:mainfrom
mrkillbob:codex/protected-worker-capability-20260826
Open

mrkillbob wants to merge 27 commits into
NousResearch:mainfrom
mrkillbob:codex/protected-worker-capability-20260826

Conversation

@mrkillbob

Copy link
Copy Markdown

Summary

  • enforce source-bound, fail-closed egress for Codex, Nous, and Anthropic provider boundaries
  • preserve protected Kanban worker tool capability without disclosing private workspace paths
  • bind exact file slices to request identity and reject oversized or unsafe sanitized segments
  • keep guarded prompts compact while retaining autonomous Kanban terminal lifecycle

Verification

  • 558 focused tests passed, 3 skipped
  • Ruff passed on all changed Python files
  • git diff --check passed

This is separate from the PR-feedback merge-controller changes.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/tui Terminal UI (ui-tui/ + tui_gateway/) tool/file File tools (read, write, patch, search) tool/terminal Terminal execution and process management provider/anthropic Anthropic native Messages API provider/nous Nous Research API (OAuth) provider/openai OpenAI / Codex Responses API area/config Config system, migrations, profiles labels Aug 26, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related to #93182's sanitized remote-worker proposal and #90820's Kanban worker controls. This broader source-bound egress approach needs a maintainer architecture/security decision.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 19954ad7f030b2fd37962d263e3691485e945e6e against live main@74cb4cb80c7f6e6c0dfe05e079b5143bceefca35. The branch is 18 commits ahead / 8 behind its actual live-main merge base 68518c1f9bca11d9f5dbdf59ecf7e024cce057ba. I inspected the 41-file change surface with the behavior-critical path centered on agent/llm_egress_firewall.py, agent/llm_egress_runtime.py, agent/source_provenance.py, agent/tool_executor.py, tools/file_tools.py, the Codex/provider dispatch hooks, protected-Kanban prompt/spawn changes, and the new provider/runtime tests. Existing PR discussion currently contains only the architecture-triage note linking #93182 and #90820.

There is genuinely strong work here. The exact file-slice grant is carefully constructed: original spelling is checked for symlink components, the source is opened with no-follow semantics where available, hardlinks are rejected, fd/name identity is checked, the bounded slice is re-read, sensitive-path policy and redaction are re-applied, and the grant is bound to session × turn × request × policy. The provider boundary also hashes the exact serialized body and re-verifies it immediately before callback. Those are the right kinds of monotonic proof.

I do have three P1 blockers plus one hard structural gate. They are all the same broad lesson: a source-bound egress system can only be as strong as the weakest place where “this text is not source” is asserted.

P1 — arbitrary terminal-read source bytes can enter the grantless SanitizedSegment lane

The source provenance producer is deliberately activated only for read_file:

  • agent/tool_executor.py::_source_provenance_activation() returns nullcontext() unless function_name == "read_file".
  • _attach_trusted_source_provenance_metadata() has the same read_file-only condition.

But protected workers still retain terminal, and this PR does not constrain terminal from reading files (cat, sed, awk, Python, etc.). tools/terminal_tool.py only receives a description edit in this branch.

The consumer then treats absence of a matching source grant as evidence that the text is sanitized:

  • agent/llm_egress_runtime.py::_segment_text() searches the known exact grant strings.
  • If no match exists, _approved_sanitized_segments() constructs SanitizedSegment(text) after only a type/byte-cap check.
  • _segment_protected_tool_result() sends every non-recognized terminal-output span back through _segment_text().
  • agent/llm_egress_firewall.py::authorize() explicitly has a grantless remote lane when _is_strict_sanitized_only_payload() succeeds.

That inverts the provenance law. SanitizedSegment is documented as “Non-source text”, but its constructor path proves only “I did not find these bytes in one of the grants I happened to issue.” A remote worker can therefore do the equivalent of:

terminal: cat relative/path/to/internal_source.py
-> ordinary source text with no secret-shaped token, absolute path, or base64
-> no SourceGrant (terminal is not a trusted producer)
-> _segment_text() labels the output SanitizedSegment
-> grantless remote request is eligible

The heuristic secret/base64/private-path scans do not answer whether ordinary source bytes are source bytes. This is the other side of the exact-grant design: an untrusted producer cannot earn “non-source” simply because exact provenance is missing.

Required repair: make non-source origin a positive proof, not the fallback case. The cleanest composition is with the already-open sanitized-worker/container direction in #93182 and the #82591 zero-authority-worker architecture: protected remote workers should see only a controller-produced sanitized workspace/broker surface, or the terminal boundary itself must return typed, origin-aware data whose readable source bytes are grant-bound. Do not attempt to parse arbitrary shell commands into a trusted file reader. Unknown/untyped tool output should preserve untrusted_provenance rather than acquire SanitizedSegment authority.

Add a vertical regression that runs a recognized terminal call whose result is the innocent contents of an ungranted source file (no path, no secret prefix, no encoding), feeds that tool result into the next protected provider request, and proves the provider callback is never invoked.

P1 — the final firewall repeats the still-open exact-secret egress defect in #77165

agent/llm_egress_firewall.py::_contains_secret() calls only:

redact_sensitive_text(value, force=True, redact_url_credentials=True)

The branch does not wire the authoritative per-home applied-secret snapshot (get_secret_source_values() / _SECRET_SOURCE_VALUES_BY_HOME) into this final provider boundary. That is exactly the open class in #77165:
#77165

#77165 already records why shape-based redaction is insufficient: an opaque 1Password/Bitwarden/CommandSource value with no vendor prefix or recognizable credential grammar can survive redact_sensitive_text() in provider-bound tool output/sanitized context. This PR's grantless SanitizedSegment path makes that gap load-bearing because ordinary unmatched text is explicitly allowed to become remotely sendable.

The current tests cover secret-shaped token=..., private absolute paths, and canonical base64. They do not cover an exact applied secret whose bytes have no recognizable shape.

Required repair: at the final provider boundary, consume the same authoritative exact-value snapshot as the secret-loading path, scoped to the exact profile/home, plus the appropriate credential-valued environment snapshot. Every outgoing string must be checked against those exact values before the allow receipt is committed. Add a real vertical test: install one arbitrary external-secret value into the per-home snapshot, surface it through terminal/tool-result content, then prove the protected provider callback cannot receive those bytes. If this PR is intended to own that class, interlock it with #77165 and preserve that issue's lineage; otherwise #77165 is a prerequisite and this PR should not claim complete source-bound secrecy until composed.

P1 — unconditional fcntl import makes the new core path non-importable on Windows

agent/llm_egress_firewall.py imports fcntl unconditionally and _append_receipt() uses fcntl.flock().

That is not isolated to an optional Linux-only path. tools/file_tools.py now imports agent.source_provenance at module import time; agent.source_provenance imports SourceGrant from agent.llm_egress_firewall. On Windows, where fcntl is unavailable, importing the file-tool surface now fails before a protected-provider request is even attempted.

Required repair: use one portable repository lock primitive for the receipt ledger, with a Windows implementation and multi-process append semantics. A process-local threading.Lock is not equivalent because multiple Hermes/profile processes can share the receipt path. Add at minimum Windows import coverage plus concurrent append/chain-integrity coverage.

There is no exact-head hosted Windows receipt to contradict this: the current head's CI 33015630556, Docker 33015629746, and Nix 33015629732 all concluded action_required; the CI run has zero jobs, and the commit currently has zero check-runs. The local 558-test claim is useful development evidence, not cross-platform exact-object acceptance.

Hard structural gate — this security feature grows already-over-2K authority files

This branch adds behavior to agent/tool_executor.py (the patch reaches line ~2812) and tools/file_tools.py (patch reaches line ~2752). Both have explicit open fracture owners:

  • #79975agent/tool_executor.py 2K-law violation
  • #79977tools/file_tools.py 2K-law violation

The repository's standing decomposition law is monotonic: godfiles are sharded and never regrown. #82591 is even more specific for this worker-security train: behavior-neutral shard first, then add containment/policy only in extracted owners. The provenance activation/metadata code added to tool_executor.py and the trusted-read provenance code added to file_tools.py are coherent new responsibilities and natural extraction seams; they should land in sub-2K owners rather than deepen those monoliths.

Graph / ownership / merge order

  • #93182 / @mrkillbob is complementary sanitized remote-worker containment. It already owns manifest-bound sanitized workspaces, offline digest-pinned containers, and a narrow staging-only broker. This PR's source-grant/provider-byte work should compose with that boundary rather than replace it with heuristics around an unrestricted host terminal.
  • #90820 / @jrgros-ops is complementary explicit strict-readonly Kanban capability/workspace authority. It is not a duplicate egress firewall; keep its task capability ownership separate.
  • #82591 is the broader zero-authority worker + publication + reclaim architecture. Its hard invariant says whole-worker containment and no generic host terminal/network path for strict remote workers. This PR should be one egress/provenance component of that graph, not a competing containment model.
  • #77165 / @andrexibiza owns the still-open exact applied-secret egress class. Do not silently supersede it with shape-based scanning; either absorb that exact-value mechanism with credit/interlock or declare it a prerequisite.
  • #79975 / #79977 own the required decomposition seams for the two newly grown godfiles.

Commit authorship on this branch is recorded as Mike DeMott <mikedemott@Mikes-Mac-mini.local> while the PR is opened by @mrkillbob; GitHub does not currently associate that local email with an account in the commit API. Preserve Mike DeMott's authored commits exactly and make sure the repository attribution mapping is present before the final push/CI gate rather than rewriting history.

What I would keep

Please preserve the exact-source fd/range/hash verification, request-bound grant identity, exact serialized provider-body digest, immutable AuthorizedEgress, provider-callback pre-send verification, loopback/local fast path, content-free receipts, and the negative tests around base64/private paths/post-preflight mutation. Those pieces are useful and unusually concrete.

Once the non-source assertion becomes positive provenance instead of absence-of-proof, exact applied secrets are checked at the final boundary, Windows receipt locking is portable, and the new responsibilities are moved out of the godfiles, this becomes a much stronger foundation for protected remote workers. There is a lot of good security engineering here; the blockers are precisely at the few remaining authority projections. 🚀

@mrkillbob

Copy link
Copy Markdown
Author

Addressed the actionable review findings at exact head 4f103a9ae21b08337cd7cebf7c419d9dc0eb5c36 (normal push; no history rewrite).

  • Protected terminal results no longer acquire SanitizedSegment authority from a missing grant. Only fully parsed, bounded syntax atoms and separators are admitted; every other result becomes a content-free UntrustedProvenanceSegment and the firewall returns untrusted_provenance. The vertical regression feeds innocent ungranted source bytes through a recognized terminal result and verifies the provider callback is never invoked.
  • The final remote provider boundary now snapshots exact values from get_secret_source_values() for the active profile home plus credential-valued environment entries, and checks every outgoing string independently of shape-based redaction. This is explicitly interlocked with the exact-secret class in fix(security): applied-secrets snapshot not wired into provider-egress redaction (tool results, sanitized context, terminal output) #77165.
  • Receipt serialization now uses a dedicated portable OS-backed lock owner (fcntl on POSIX, msvcrt on Windows) and a separate no-follow lock file. Coverage includes import with fcntl unavailable, concurrent multi-process appends with hash-chain verification, and lock/ledger symlink refusal. This is local cross-platform contract evidence, not a hosted Windows acceptance claim.
  • Provenance activation, metadata, and trusted-read grant construction moved into agent/source_provenance_tools.py; agent/tool_executor.py and tools/file_tools.py now retain integration calls rather than the new authority responsibilities. The portable lock is also isolated in agent/cross_process_file_lock.py.
  • Existing authored commits remain unchanged. .mailmap now maps Mike DeMott <mikedemott@Mikes-Mac-mini.local> to 25466867+mrkillbob@users.noreply.github.com.

Fresh exact-head evidence: 777 passed, 3 skipped across every test file changed by the PR; the focused security/file-tool set is 196 passed, 2 skipped; Ruff passed on all touched Python; git diff --check passed.

This remains an egress/provenance component that composes with #93182, #90820, and #82591; it does not claim to replace their containment or capability ownership.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 4f103a9ae21b08337cd7cebf7c419d9dc0eb5c36 against live main@7a7a371c593b8956f8d6f0bb588d2d6afdc8d428. The branch is 19 commits ahead / 24 behind its actual merge base 68518c1f9bca11d9f5dbdf59ecf7e024cce057ba; I found no changed-path overlap in the current live-main delta. I inspected the repair commit, the full source→tool-result→typed-request→provider callback path, receipt emission, tests, discussion, and the #93182 / #90820 / #82591 / #77165 interlocks.

The repair does close several findings from the prior head: the normal exact-secret snapshot path is now wired at the provider boundary, provenance and lock responsibilities were extracted into small owners, the unconditional fcntl import is gone, and the attribution mapping is present. Three P1 defects remain, plus the exact-object verification gate.

P1 — the real read_file wire result loses its grant and becomes grantless SanitizedSegment

The new provenance tests validate the grant producer and the runtime tests validate a synthetic request containing the raw granted bytes, but the production presentation path transforms those bytes before the provider sees them:

  1. tools/file_operations.py::_add_line_numbers() prefixes every returned source line with N|.
  2. tools/file_tools.py::read_file_tool() validates that line-numbered presentation, then issue_active_read_provenance() issues a grant over the original raw file bytes.
  3. read_file_tool() returns json.dumps(result_dict), and agent/tool_executor.py appends that transformed JSON string as the tool message content through make_tool_result_message().
  4. agent/llm_egress_runtime.py::_segment_text() recognizes a grant only when the exact raw grant text occurs in the outgoing string. The line-numbered, JSON-escaped tool result does not contain that exact byte sequence, so matches is empty and _approved_sanitized_segments() assigns SanitizedSegment authority to the whole result.
  5. attach_trusted_source_provenance_metadata() writes opaque digests to agent._source_provenance_metadata, but this branch has no consumer for that metadata; the only source_grant_digests references in the patch are the producer and its unit test. authorize_agent_sdk_kwargs() passes only used_grants, which therefore remains empty.

That means an ordinary bounded read_file result can cross a remote boundary with source_grant_count == 0 and source_segment_count == 0. The source bytes were read by the trusted producer, but the proof is detached before the actual provider request is constructed. This is the inverse failure of the original terminal bug: the grant exists, yet the presentation transform launders the source into the grantless lane.

Required repair: carry typed source references through the actual tool-result envelope rather than rediscovering them by exact substring search after line numbering and JSON serialization. The N| gutters and JSON structure can be literal/sanitized framing, but the underlying file text must render from SourceBoundSegment references tied to the exact request grant. Consume the opaque metadata at that boundary or replace it with a typed message object; do not leave it as write-only state.

Add one vertical regression using the real production path: execute read_file_tool() under source_provenance_activation(), build the real tool message with make_tool_result_message(), feed it into authorize_agent_sdk_kwargs(), and assert that the callback receives the intended line-numbered presentation with a nonzero bound grant/segment count. The same test with missing, stale, or forged grant metadata must fail closed.

P1 — terminal stdout can still launder source bytes by matching the syntax grammar

_segment_protected_tool_result() now avoids the old generic sanitized fallback, but it explicitly discards grant_texts and used_grants and upgrades any terminal output composed entirely of _TOOL_SYNTAX_TOKEN atoms plus separators into ValidatedToolSyntaxSegment nodes. The allowlist includes GitHub URLs, git refs, repository/issue identifiers, and arbitrary 40- or 64-character lowercase hex strings. LLMEgressFirewall._is_strict_sanitized_only_payload() counts those nodes as positive grantless text, and the current positive test explicitly authorizes:

https://github.com/acme/widget.git
run_id=1129 --force-with-lease refs/heads/codex/fix-135

A workspace file containing exactly that text—or only a commit SHA—produces indistinguishable stdout when read with cat. The current regression blocks a Python function body, but it does not cover source bytes that happen to satisfy the grammar. Syntax proves shape, not origin; arbitrary terminal stdout cannot earn non-source authority by parsing successfully.

Required repair: never construct ValidatedToolSyntaxSegment from generic terminal stdout. That type can be emitted only by a trusted structured producer that owns the value—for example, a narrow controller/broker result with explicit fields—not by reparsing an unrestricted shell’s text stream. Unknown terminal output remains UntrustedProvenanceSegment unless a separate trusted producer binds its source.

Add adversarial vertical cases where a recognized terminal call reads files containing only (a) an allowed GitHub URL, (b) an allowed ref, and (c) a 40/64-character hex value. Each must leave the provider callback uninvoked. This remains the key composition issue with #93182, #90820, and #82591: an unrestricted host terminal cannot simultaneously serve as an untyped producer and a source-authority boundary.

P1 — receipt emission still crashes on supported Windows Python 3.11/3.12

The new exclusive_file_lock() owner is portable, but LLMEgressFirewall._append_receipt() still calls:

os.fchmod(fd, 0o600)

unconditionally. This repository declares Python >=3.11,<3.14. Python 3.12 documents os.fchmod as Unix-only, and Windows support was added in Python 3.13:

On Windows 3.11/3.12, the missing attribute raises AttributeError, not OSError. Both receipt callers catch only OSError, so both allowed requests and blocked requests can escape as an unrelated crash instead of returning an authorization or EgressBlocked decision. The existing “fcntl unavailable” import test and POSIX multiprocess ledger test do not execute this path.

Required repair: move the descriptor-permission operation into the portable receipt owner and guard/use a Windows-appropriate implementation while preserving secure creation semantics. Add a test that removes os.fchmod from the platform surface and exercises both an allow receipt and a block receipt end to end; neither path may raise AttributeError, and the ledger/hash chain must remain valid.

Exact-object verification gate

At this exact head, CI 33021225856, Docker 33021225182, and Nix 33021225196 all concluded action_required; the CI run has zero jobs and the commit has zero check-runs. The PR body’s local 558 passed, 3 skipped, Ruff, and git diff --check report is useful development evidence, but it is not hosted exact-head acceptance and does not establish the Windows path or a green 19-commit train.

The fd/range/hash source verification, immutable serialized provider payload, pre-callback digest verification, exact-secret normal path, and extracted ownership are worth preserving. This head is not merge-ready until the two provenance bypasses are closed, Windows 3.11/3.12 receipt emission is exercised, and CI/Docker/Nix execute successfully on the exact commit.

@mrkillbob

Copy link
Copy Markdown
Author

Addressed the three exact-head P1 findings in 48467f7bf88 (normal push; no history rewrite).\n\n- Real read_file results now carry an internal content-bound provenance envelope through make_tool_result_message(). The provider boundary strips that metadata, verifies the exact request/grant/content hash, reconstructs the deterministic line-numbered JSON presentation as a typed SourcePresentationSegment, and revalidates it against freshly read grant bytes. Missing, stale, or forged envelopes fail closed as untrusted_provenance.\n- Generic terminal stdout can no longer create ValidatedToolSyntaxSegment authority by matching a URL/ref/SHA grammar. Every generic terminal result remains untrusted; adversarial URL, ref, 40-hex, and 64-hex file-content cases all block before the provider callback.\n- Receipt descriptor hardening moved into the portable lock owner. Python 3.11/3.12 Windows paths without os.fchmod now preserve allow/block decisions and the receipt hash chain instead of raising AttributeError.\n\nFresh exact-head local evidence: all 17 PR-touched test modules passed (785 passed, 3 skipped); Ruff passed on all changed production/test files; git diff --check passed. Hosted CI/Docker/Nix execution remains repository-authority evidence, not claimed by this local run.

@mrkillbob

Copy link
Copy Markdown
Author

Implemented the exact-head transport/tool-loop repair. read_file provenance now crosses strict message conversion only in a bounded content-free internal sidecar, is removed before provider dispatch, and is re-authorized on later API calls only after exact session/turn/policy/original-grant identity and canonical source bytes are revalidated. Missing, forged, stale, or mutated presentations remain fail-closed; generic terminal output remains untrusted.

Evidence at 78e865f956429a0c77f03dfee81fbe597fe397bc:

  • focused provenance/transport slice: 61 passed
  • full touched suite: 384 passed (one existing thread-fixture warning)
  • Ruff: passed
  • git diff --check: passed

intent-review: 5035395685 use alternative

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of exact head 78e865f956429a0c77f03dfee81fbe597fe397bc against the blockers in review 5035395685.

The four follow-up commits materially close the source findings I raised:

  • Unknown terminal output no longer earns sanitized authority. Generic protected terminal stdout is represented as UntrustedProvenanceSegment; the vertical regression feeds innocent ungranted source bytes from a cat-shaped terminal result through the real provider boundary and proves the callback is not invoked.
  • Exact applied-secret values are now checked at the final provider boundary. _exact_provider_secret_values() snapshots the active home's get_secret_source_values() plus credential-valued environment entries and supplies them to the firewall. The regression uses an opaque purple-lantern-river-cobalt source value with no credential-shaped grammar and proves exact_secret_detected before dispatch.
  • The receipt lock is portable. The unconditional fcntl dependency is gone; agent/cross_process_file_lock.py uses OS-backed flock on POSIX and msvcrt.locking on Windows. The tests include simulated no-fcntl import plus multi-process append/hash-chain integrity.
  • The godfile growth was reversed into narrow seams. Relative to the reviewed head, the repair removes 82 lines from agent/tool_executor.py and 69 from tools/file_tools.py, moves the provenance policy/adapters into the new 198-line agent/source_provenance_tools.py, and leaves the two large owners with small integration calls rather than the new policy implementation.

I also checked the new transport/tool-loop repair: the read-file provenance envelope is content-bound, kept as internal sidecar metadata, stripped from provider payload, and later rebind requires the original grant plus exact session/turn/policy/request identity and current source bytes. Missing/forged/mutated presentation degrades to untrusted provenance.

I do not see a remaining version of the four source blockers from my prior review on this head.

Two landing gates remain before this object is merge-ready, and neither is a substitute for the source review above:

  1. The exact-head CI 33039328011 is action_required; Docker and Nix are likewise not executed on this head. There is therefore still no hosted exact-object matrix for 78e865f….
  2. Live main is now 1a66134404b891170e953f51662e6429f0b7b5a9, 118 commits beyond this PR's base. The landing interval intersects three submitted paths (hermes_cli/config_defaults.py, tools/terminal_tool.py, tui_gateway/server.py). GitHub currently reports the PR mergeable, but those owner-path changes still require a final semantic landing-edge read/rebase and fresh exact-head CI rather than transferring this head's local receipts.

Source-review status: prior blockers closed. Landing/acceptance status: waiting on exact hosted execution plus current-main reconciliation.

@mrkillbob

Copy link
Copy Markdown
Author

Landing-edge reconciliation done at 01ed2661199.

Thank you for the re-review — taking the two remaining gates in order.

Gate 2, current-main reconciliation. The branch was 179 commits behind main by the time I picked this up (a588685fb95), not 118, so I merged current main in with --no-ff rather than transferring the old head's receipts. It merged with zero textual conflicts, but a clean merge is not the claim you asked for, so I checked the three landing paths semantically:

path lines this PR touches lines main touches in common
hermes_cli/config_defaults.py 20 36 0
tools/terminal_tool.py 2 73 0
tui_gateway/server.py 6 489 1

The single shared line in tui_gateway/server.py is a closing ) — incidental, not a semantic collision. The two sides edit disjoint regions of all three files.

Tests on the merged head: tests/agent/test_llm_egress_firewall.py + tests/agent/test_llm_egress_runtime.py 126 passed; tests/run_agent/test_run_agent.py 281 passed. All three landing-edge modules still resolve as importable specs.

Gate 1, exact-head CI. Still outside what I can settle from here: the workflows on this fork head need a maintainer to dispatch them, so there is still no hosted exact-object matrix. Flagging it rather than claiming it — the local runs above are development evidence, not hosted acceptance, exactly as you framed it.

No source changes in this push; it is purely the landing-edge merge plus verification.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 01ed26611997172c0133f17f682c4a9027e211b6 against the response in comment 5438178911, the merge parents, live main@5fc308a70719a83cccdbba4c0e39c23f5a8239d5, and the two remaining gates from review 5037599205.

The landing-edge source gate is closed. 01ed266… is the stated no-ff merge of reviewed source head 78e865f… with main@a588685…. I checked the final merged seams rather than transferring the author's line-count claim:

  • hermes_cli/config_defaults.py preserves the PR's reasoning, guarded-prompt, and reviewer-profile defaults alongside main's additions.
  • tools/terminal_tool.py remains a description-only change at the foreground-timeout schema surface.
  • tui_gateway/server.py preserves the source-provenance turn establishment, kwargs binding, and blocked-path cleanup around preprocess_context_references().

The reported merged-head 126 passed egress slice and 281 passed run-agent slice are consistent with that composition. I do not see a source or merge-resolution defect in the reconciled object.

I also checked movement after that reconciliation. Ten commits now sit beyond the current pull-request merge ref's base parent a9611f3…. Only agent/auxiliary_client.py and tests/agent/test_auxiliary_client.py overlap this PR, through a65ad156… (fix(agent): honor explicit free OpenRouter models). That patch forwards an explicit OpenRouter model into the free_only gate and adds its resolver regressions. This PR's new _AUX_EGRESS_PROVIDERS authority is explicitly limited to anthropic, openai-codex, and nous; OpenRouter bypasses that protected dispatch, and the tests occupy separate behavior. This is semantic/path-disjoint drift, so I am not asking for another churn-only restack.

One exact-object acceptance gate remains. At 01ed266…:

  • CI 33066632592: action_required, zero jobs
  • Docker 33066631850: action_required, zero jobs
  • Nix 33066631849: action_required, zero jobs
  • commit check-runs/status contexts: zero

Those created workflow objects reference pull-request merge object 3b1fd090…, whose base parent is a9611f3…, not current main@5fc308a…. Approving or rerunning that stale merge object would not establish the final current-main release object.

Disposition: all prior source blockers remain closed, and current-main semantic reconciliation is accepted. This is now solely a repository-authority acceptance gate: execute CI, Docker, and Nix successfully on one refreshed exact current-main merge object. No further source change is requested unless that execution exposes one.

Mike DeMott added 2 commits August 28, 2026 01:19
…r-capability-20260826

# Conflicts:
#	agent/auxiliary_client.py
…20260826' into codex/protected-worker-capability-20260826
@mrkillbob

Copy link
Copy Markdown
Author

Merge conflict resolved: composed current main (180 commits behind) onto this branch. One conflict in agent/auxiliary_client.py — this branch's scoped_runtime_main(main_runtime) context manager (unrelated to this refactor) needed re-adding alongside upstream's now-much-larger latency-tracking with (...) block (aux_progress_hook, _aux_timing_hook x2) that had grown around it across the commit gap. Everything else auto-merged.

Resolved head 382fe2a420aef0f51d39d55a69dd117badd20f09. Focused verification against the files this PR touches: 517 passed, 1 skipped, 1 failed — the one failure (TestStaleBaseUrlWarning::test_warns_when_openai_base_url_set_with_named_provider) is ModuleNotFoundError: No module named 'openai', an optional-dependency gap in this sandbox (not installed by the base uv run), not a merge regression — confirmed by re-running the rest of that same file's suite clean (198 passed) after excluding just that one dependency-gated class.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head 382fe2a420aef0f51d39d55a69dd117badd20f09 after the main refresh and the hand-resolved agent/auxiliary_client.py conflict.

The prior source blockers remain closed on this object. The egress runtime still derives the firewall secret set from the exact applied secret names/values rather than an ambient credential pool; protected terminal stdout still requires self-provenanced registered read-launch authority, while diagnostics, missing/forged launcher provenance, and grant reuse remain rejected; and cross_process_file_lock.py still selects msvcrt.locking on Windows and fcntl.flock on POSIX. The corresponding current-head tests remain present, including exact applied-secret scoping, self-provenanced terminal reads, forged/missing terminal authority rejection, one-shot grant consumption, and the Windows portable-lock path. Mike DeMott's contributor mapping is also present.

I specifically re-read the one hand-resolved conflict. scoped_runtime_main(main_runtime) is composed into the same provider-call context as upstream's aux_progress_hook and both timing hooks, rather than replacing or bypassing those upstream additions, so the merge resolution does not reopen the protected-runtime seam.

Landing-edge drift is currently bounded: live main@5f75ec197b16d1a055688fb9df63ac49586a2230 is exactly one commit beyond this PR's base 48d25280669f645550c86b7540c01996f611be63, and that main-only commit touches the code-execution kernel/tests/docs surface, not this PR's security/runtime paths. No new source change requested from this review.

Acceptance is still not complete. Exact-head Docker 33154910377, Nix 33154910413, and CI 33154911140 all terminate action_required without executing the hosted acceptance matrix. I attempted the reversible failed-run rerun for CI 33154911140; GitHub rejected it with 403 Resource not accessible by integration. The author's focused local results are useful development evidence, but they cannot substitute for exact-object hosted execution or establish the every-surviving-commit-green landing gate. Do not merge this head until the required workflows actually execute green.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

This diff is too large for an automated line-level review (>30 files or >200KB). Recommend a manual review.

@mrkillbob

Copy link
Copy Markdown
Author

Integrated local-only fallback routing from PR #97370 into this PR.

Source PR commit: a3dd3f8
Integrated commit / verified remote head: 15e43f1
Focused tests run: python -m pytest tests/run_agent/test_provider_fallback.py tests/agent/test_error_classifier.py
Focused source-bound egress/provenance/firewall suite run: python -m pytest <all git-tracked tests matching egress, provenance, or firewall>
Both test commands passed locally.

…ithub-pr-feedback/1746d92e7f9c3794ce748cb1222cb026c91b9dd462c24bdd74c68c477301d1f5

# Conflicts:
#	agent/auxiliary_client.py
#	agent/chat_completion_helpers.py
#	agent/codex_runtime.py
#	agent/coding_context.py
#	agent/context_references.py
#	agent/conversation_loop.py
#	agent/error_classifier.py
#	agent/kanban_stop.py
#	agent/prompt_builder.py
#	agent/system_prompt.py
#	agent/tool_executor.py
#	cli.py
#	hermes_cli/cli_agent_setup_mixin.py
#	hermes_cli/config_defaults.py
#	hermes_cli/dump.py
#	hermes_cli/kanban_db.py
#	run_agent.py
#	tools/file_tools.py
#	tools/kanban_tools.py
#	tui_gateway/server.py
@mrkillbobbot

Copy link
Copy Markdown

Updated the PR with a normal base-refresh merge of 245e480. The pushed head is 91794ad. Local compileall passed (with existing SyntaxWarning output); targeted pytest could not run because the configured Python reported No module named pytest.

@mrkillbob

Copy link
Copy Markdown
Author

@Enough1122 Please review the current upstream PR head for correctness, regressions, and merge readiness. This request is specifically for your AI review; do not route it to Codex.

@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/tui Terminal UI (ui-tui/ + tui_gateway/) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have provider/anthropic Anthropic native Messages API provider/nous Nous Research API (OAuth) provider/openai OpenAI / Codex Responses API sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/file File tools (read, write, patch, search) tool/terminal Terminal execution and process management type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants