Repository navigation
docs(isolation): add ADR 0004 for neutral model-provider discovery - #423
Conversation
Records the design that emerged from three rounds of nexus-agents 7-role consensus review this session (7-0 reject on live-poll-with- no-discovery; 4-3 approve/reject split on host-side third-party code execution next to secrets; 5-2 approve on in-guest polling that reuses the harness's own already-standing network+credential trust boundary), plus follow-up research into the actual duplication this solves: - acq's secret store dedupes by service-name string only, never by host: usai-provider registers USAi's credential as service `usai`; the in-flight goose-server kit independently registers the SAME credential, same host, as service `goose-usai`. Confirmed via acq.backends/secret-store.sh's _acq_secret_key. - The two kits' copy-on-first-boot/merge-without-clobbering config scripts are ~230-260 lines each, ~150-190 of which per kit are the same algorithm, hand-copied (goose's own header admits it mirrors usai-provider's). - The bounded-fetch-with-timeout-and-size-cap helper has already been copy-pasted once and has already drifted (the copy gained 401/403 handling the original lacks). Decision: three-layer design — (1) provider kits self-register via a well-known guest-path facts file (mirrors the already-shipped zscaler-ca-certificate -> openchamber/paseo pattern) and ship their OWN normalizer + vendored fallback snapshot; (2) a new, vendor- agnostic models orchestrator kit (pure orchestration, zero vendor knowledge, core-repo promotion candidate) discovers providers, refreshes a host-mounted-volume cache on a TTL, falls back to last-good-cache then to the provider's own snapshot on any failure, and aggregates into one neutral catalog; (3) each harness kit reads that aggregate + config-kit defaults and renders its OWN native config, owning its full configuration lifecycle end-to-end. N provider normalizers + M harness renderers, not NxM per-pair emitters. Incorporates both dissenting votes from the final round as binding conditions: an env-var-ownership check (a provider kit cannot name a credential env var it doesn't itself declare) and SSRF hardening (fetched host must equal the facts file's own declared host, itself on the sandbox's already-effective network allow-list). No hybrid/v1 schema change required — composes entirely from already-shipped primitives (caps.network.allow union, sequential kit-startup ordering, well-known-guest-path self-registration, the neutral volumes[] vocabulary for the cache mount). Co-authored-by: OpenCode Agent <agent@gsa.gov>
|
CI-green, mergeable, no other blockers I can see. I can't self-approve (author). Could a maintainer review/merge this ADR when convenient? I have follow-up implementation work (#424) that cites this ADR's path — low risk either way since it's docs-only, but merging unblocks a clean citation rather than a provisional one. |
There was a problem hiding this comment.
See note; agents have passwordless sudo, so root-ownership is not the barrier implied. It's possible to mount a volume :ro, but we can't do that if we want the kit startup to be how we drop a file there. Other ideas? Maybe acq is responsible for grabbing the file from the kit and putting it there, and it's :ro?
|
Also: This clarity of this ADR would be vastly improved by the addition of Mermaid sequence diagrams. |
…ed cache Corrects ADR 0003 after PR #423 review (mogul): the Layer 1 provider-facts mechanism relied on a root-owned guest path being un-writable, but acq grants the in-sandbox agent passwordless sudo, so guest root-ownership is not a trust boundary against a prompt-injected agent. It could sudo-write a bogus provider facts file that the models orchestrator would then trust. Changes: - Layer 1: provider kits SHIP a static facts file in their (pinned) kit dir; acq reads it on the host, validates it host-side (env-var-ownership + SSRF + schema), and mounts it read-only into the guest. No provider-kit code runs on the host (static data read only); the normalizer still runs in-guest. - Layer 2: drop the shared host-side cache + TTL entirely. A cache written by one sandbox and read by another is a cross-sandbox poisoning channel. The catalog is generated per-sandbox at startup, kept guest-local (rw), and never shared; the deepest fallback is the provider's vendored snapshot on the read-only mount. - Add a note on startup integrity across restarts (orchestrator/normalizer code invoked from the read-only mount so it cannot be tampered between restarts). - Add two Mermaid sequence diagrams (GitHub/Host/Guest boxes) contrasting the rejected guest-write path with the chosen host-materialized read-only path. - Reference quickstart ADR-0030 (Host-Authoritative Sandbox Configuration) as the general principle and mount/exec mechanism of record. Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
|
Pushed The security issue you flagged is removed. The Layer 1 mechanism no longer relies on a root-owned guest path being un-writable. You're right that this isn't a barrier: acq deliberately gives the in-sandbox agent passwordless sudo, so a prompt-injected agent can Layer 1 — host-authoritative, guest read-only. Provider kits now ship a static facts file in their (pinned) kit directory rather than writing one from a Layer 2 — shared host cache dropped. Per your call, the cross-sandbox host cache + TTL is gone entirely: a cache written by one sandbox and read by another is a cross-sandbox poisoning channel. The catalog is now generated per-sandbox at startup, kept guest-local (rw), and never shared. Deepest fallback is the provider's vendored snapshot on the Your restart-tamper concern is addressed too. You noted the models kit startup script could be tampered between restarts. Correct — and it's the general form of the same problem, since acq's restart heal re-runs guest-resident kit code. The fix is the same boundary: the orchestrator/normalizer code is staged into the General principle recorded separately. The mount/exec mechanism and the broader rule ("acq must not trust guest-generated/tamperable state to configure the sandbox; migrate the existing Mermaid diagrams added (your other request): two markdownlint-cli2: 0 issues. |
The ADR had accreted into a change-log of the review (consensus rounds, vote tallies, "corrected from an earlier draft" notes, reviewer names, dueling Option 2/3 correction blocks). Once merged, an ADR is documentation of how the thing works, not a journal of how we arrived at it. Rewrite it to describe the agreed end state directly: - Context states the problem (duplicated credential registration, duplicated merge code, manual release-coupled refresh) and the design goals, without the panel/vote narrative. - Decision describes Layers 1-3 in the present tense as the design, folding the host-authoritative read-only facts/code rationale inline instead of as a post-hoc correction. - A single "Security model" section replaces the scattered "hardening conditions" attributed to reviewer roles. - One "Trust-boundary flow" diagram (the chosen design); the rejected guest-write sequence diagram is dropped (its point now lives as one line in Alternatives + the Layer 1 rationale). - Alternatives Considered keeps WHAT was excluded and WHY, dropping vote counts and session references. No behavioral change; documentation only. Co-authored-by: OpenCode [claude_4_8_opus] <bret.mogilefsky@gsa.gov>
|
Pushed Your point is right: once this merges, the ADR is reference documentation. The prior revision had turned into a review journal — consensus rounds, vote tallies (7-0 / 4-3 / 5-2), reviewer role names, "corrected from an earlier draft" blocks, and two separate "the guest-write approach was wrong" correction notes. A reader arriving after merge has to reconstruct the actual design from the diffs between rejected and accepted states. What changed:
No design change from markdownlint: 0 issues; the remaining Mermaid diagram parses (mermaid v11). (The provenance you'd normally want preserved — the consensus rounds and the review that caught the sudo/root-ownership flaw — lives durably in this PR's history and conversation, which is the right home for it.) |
… prose-only Adds the actual quickstart#506 link to the References section — the prior commit named the issue in prose but didn't link it since #506 wasn't filed yet at write time.
|
I'm taking on GSA-TTS/agentic-coding-quickstart#506 so we can merge this one without caveats about that race-condition. |
|
`integrations/isolation/docs/decisions/0003-` was claimed by two open PRs: #416 (serviceGateways vocabulary, opened 2026-09-15) and this one (opened 2026-09-18). The filenames differ, so git merges both cleanly and leaves two ADR-0003s in the same directory — every later "ADR 0003 (isolation)" reference becomes ambiguous. #416 claimed it first and its number is not cited anywhere else, so this PR moves. 0004 is free: `main` has 0001 and 0002, and no other open PR claims it. Only the file name and the H1 change. The "usai-provider ADR 0003" reference at line 353 is a DIFFERENT, kit-local ADR under acq-kits/usai-provider/docs/decisions/ and is deliberately left alone. AI-assisted (OpenCode). Human review and merge still required.
#423's ADR moved from `0003-neutral-model-provider-discovery.md` to `0004-` to clear a collision with #416, which claimed `integrations/isolation/docs/ decisions/0003-` first. Update the two comment references here so they do not dangle once that lands. Comment-only; no behavior change. `node --check` and a YAML parse both pass. AI-assisted (OpenCode). Human review and merge still required.
wz-gsa
changed the title
Corrections found by a 5-role pr_review panel plus independent verification (acq internals, cross-repo PR status), then confirmed via a 7-role higher_order consensus vote (85.7% approve) on the fix approach itself: - Consequences/Positive claimed "one shared config-merge/bounded-fetch library replaces the per-kit copies" — false against the ADR's own Decision. Layer 3 mandates each harness kit render its own native config; only the bounded-fetch helper is actually shared. Corrected the claim and stated explicitly that no shared config-merge library exists. - Added a security invariant, in three places (Layer 2 output description, Layer 3 read-list, Security model bullet list): the guest-writable aggregate catalog.json carries only model metadata, never routing/credential fields (host/baseUrl/modelsUrl/keyEnv). Those fields are read exclusively from the Layer 1 read-only facts mount. Without this, a sudo-capable in-guest agent could rewrite the catalog to pair one provider's keyEnv with another provider's allowlisted host, bypassing the Layer 1 env-var-ownership and SSRF checks entirely within caps.network.allow. - "quickstart ADR-0030" and "goose-server ADR 0004" were cited as settled sibling ADRs. Both are, as of this commit, open and unmerged (GSA-TTS/agentic-coding-quickstart#504, #415 — freshly reverified via `gh pr view` before this commit). Added "(pending — org/repo#N)" qualifiers per the repo's fully-qualified cross-repo reference convention. Consensus panel also flagged the Layer 1 artifact-location schema gap (no field locates a provider's normalizer or vendored snapshot) as a design change, not a textual fix, and recommended landing it in its own commit for separate review. That follows in the next commit. markdownlint: 0 issues. Co-authored-by: OpenCode Agent <agent@gsa.gov>
…der directory
Design amendment, landed separately from the accuracy/citation fixes
in the prior commit per the consensus panel's recommendation (7-role
higher_order vote, 85.7% approve): several voters flagged this as a
schema change to the Layer 1 contract, not a wording fix, and asked
that it be independently reviewable/revertable from the citation and
claim corrections.
The gap: the v1 facts file has no field locating a provider's
normalizer or vendored snapshot, yet Layer 2 steps 2 and 4 require the
orchestrator to resolve both. Two independent provider-kit authors
implementing v1 as written could place those artifacts at different
conventions, and the orchestrator would find neither — undermining the
ADR's own central interoperability claim ("a third-party provider kit
works with no PR into the orchestrator").
Fix: replace the single per-provider JSON file with a fixed
per-provider directory (facts.json + normalizer + snapshot.json),
materialized read-only as a unit. Updated Layer 2's glob (step 1) and
its normalizer/snapshot resolution (steps 2 and 4) to locate both
artifacts as siblings of the facts.json just read. Layer 3's read-list
(added in the prior commit) is updated to the corrected path.
The mermaid trust-boundary diagram is left unchanged: its RO
participant description and the one path-like string in the diagram
(`providers/evil.json` in the adversary-attempt line) don't cite the
specific single-file convention this commit changes, so nothing there
needed to move.
markdownlint: 0 issues. Content verified against the full draft
produced during review (byte-identical diff, confirmed before commit).
Co-authored-by: OpenCode Agent <agent@gsa.gov>
Independent review + fixes (AI-assisted, OpenCode)Ran a 5-role Before applying any fix, ran the proposed fix set through a 7-role Commit 1 — accuracy + a security invariant (
Commit 2 — Layer 1 artifact restructure (
markdownlint: 0 issues on both commits. Full panel transcripts and the verification subagent's findings available on request. Human review and merge still required — this is advisory, not a self-approval. |
…versions Corrects an error in my own earlier revision of this ADR (e322cb0), found while trying to write the formal JSON Schemas for #435. THE DEFECT. The ADR said both a normalizer's output and the orchestrator's aggregate carry `schemaVersion: "acq-neutral-model-catalog/v1"`, AND that per-entry provenance is required on that shape. Those two statements cannot both hold: - A normalizer CANNOT know its own provenance. It is a pure file-to-file transform over bytes the bounded-fetch helper already retrieved (Layer 2, step 2) -- whether those bytes came from a live refresh or from a vendored snapshot is known only to its caller. Requiring `source: live|snapshot` in its output means a field the producer must be TOLD and cannot verify. - It would have invalidated artifacts already shipping in this shape: the normalizer output and vendored snapshot on #436 both carry this version with no provenance, and #436 is green awaiting re-review. A schema honoring the ADR literally would have failed both. THE FIX. Two documents, versioned separately: acq-neutral-model-catalog/v1 one provider's normalized model list (a normalizer's output, or a vendored snapshot) acq-neutral-model-aggregate/v1 the multi-provider aggregate (orchestrator) Provenance belongs where the knowledge is: the orchestrator performed the fetch or took the fallback, so the orchestrator records it. The aggregate requires discovery provenance (per provider: live / snapshot / rejected -- so a provider rejected by host-side validation is visible as rejected, not merely absent), and each entry names its origin `providerId`. The per-provider document requires no provenance field at all. The empty-array rule is kept on both: a provider legitimately exposing no models is valid, and an aggregate with zero entries is what step 4 writes when no provider is present. Also stated explicitly on the per-provider shape, because it is now a document that may be read from a guest-writable path: model METADATA only, never routing or credential fields (host, baseUrl, modelsUrl, keyEnv). A rewritten routing field could redirect a consumer; those facts live in the host-validated facts file. #436 already enforces this with a test asserting the shipped snapshot carries none of those keys -- this records the rule the test checks. The mistake is written into the ADR rather than quietly corrected, because the reasoning ("a pure transform cannot attest to where its input came from") is easy to re-make and the note is what prevents that. The Alternatives overlap note is updated too: there are now two distinct neutral-catalog lineages here plus the already-shipped `usai-model-catalog/v1` in integrations/providers/usai/, which makes the #435 reconciliation broader, not narrower. Operator decision, recorded: split chosen over "provenance required everywhere" once the cost of the latter was measured -- reopening a green PR and adding an unverifiable caller-asserted field to a pure function. No design change to Layers 1-3; the containment contract from e322cb0 stands. Verified: python3 scripts/validate_repo.py --root . -> exit 0, all validators passed markdownlint-cli2 -> 0 errors long-line check -> only the new 2-row table exceeds 80 git diff --check -> clean AI assistance disclosed: prepared by an AI agent (OpenCode) on operator instruction. Human review and merge still required.
|
One more correction, in The defect I introducedMy A normalizer cannot know its own provenance. It is a pure file-to-file transform over bytes the bounded-fetch helper already retrieved — whether those came from a live refresh or a vendored snapshot is known only to its caller. Requiring And it would have broken what is already shipping. Measured against #436's artifacts: A schema honoring the ADR literally would have failed both files #436 ships — a PR that is currently green and awaiting your re-review. The fix: two documents, versioned separately
Provenance belongs where the knowledge is: the orchestrator performed the fetch or took the fallback, so the orchestrator records it — per provider, as The empty-array rule is kept on both shapes, for the step-4 reason. I considered requiring provenance everywhere and amending #436 instead. The cost was reopening a green PR to add a field its producer cannot honestly supply, so the split is the cheaper and more truthful option. The mistake is written into the ADR rather than quietly corrected, because the reasoning — a pure transform cannot attest to where its input came from — is easy to re-make, and the note is what prevents that. One addition while I was thereThe per-provider shape now explicitly states model metadata only, never routing or credential fields ( Consequence for #435The overlap note in Alternatives is updated: there are now two distinct neutral-catalog lineages here, plus the already-shipped Two related things measured while writing this, recorded there rather than here:
VerificationWhere this leaves the PRAll three of your Address now items remain addressed; this corrects a defect in my fix for item 3, not a new requirement. No design change to Layers 1–3 and the containment contract stands. I am deliberately not writing the #435 schemas until this split has your eyes on it — they would be written against a version boundary that could still change. #436 needs no change from this. AI-assisted (OpenCode), on operator instruction. Advisory; human review and merge still required. |
ADR-0030 was double-claimed. GSA-TTS/agentic-coding-quickstart#474 keeps ADR-0030 because patterns main already cites it for the devenv ADR. Co-authored-by: OpenCode [gpt-5.5] <bret.mogilefsky@gsa.gov>
mogul
left a comment
mogul
left a comment
There was a problem hiding this comment.
Looking good. This is mostly commentary on minor points in the substance, rather than any quibble with what we've landed on. With these changes accounted for, I would happily approve it.
#423's ADR moved from `0003-neutral-model-provider-discovery.md` to `0004-` to clear a collision with #416, which claimed `integrations/isolation/docs/ decisions/0003-` first. Update the two comment references here so they do not dangle once that lands. Comment-only; no behavior change. `node --check` and a YAML parse both pass. AI-assisted (OpenCode). Human review and merge still required.
…al catalog Rescopes this PR to the half that is sound. Addresses both blocking items from review at a7dec19. BLOCKER 1 -- the facts file is gone. spec.yaml wrote /var/lib/acq/models/providers/usai.json from inside the sandbox at startup as root (58 lines removed), on the theory that a root-owned guest path would be a trustworthy source for a future provider-discovery consumer. That reasoning was wrong, and I verified it rather than taking it on the reviewer's word: quickstart's acq itself writes `printf "agent ALL=(ALL) NOPASSWD:ALL\n" > /etc/sudoers.d/90-acq-agent` (acq.backends/msb.sh at origin/main), and docs/BACKEND_GUIDE.md states the same guarantee as the sbx base-image contract. So the spec's own comment -- "the agent (uid 1000) cannot write concurrently (root-owned, agent has no write access to /var/lib/acq/)" -- was DAC-true but FALSE as a security property: `sudo tee` defeats it. The file advertised `keyEnv`, a credential POINTER, so an agent able to rewrite it could have redirected a consumer at a different env var. That is a confused-deputy primitive, and it was shipping with a comment licensing a consumer to trust it. Trustworthy facts need a host-materialized, read-only source. That mechanism does not exist yet -- it depends on acq's host-config `:ro` mount, still unmerged upstream -- so this kit now ships NO facts file rather than one at the wrong trust level. Rather than delete silently, spec.yaml's header records what was tried, why it was wrong, and what the replacement requires, so the next reader does not reinvent it. BLOCKER 2 -- verify check 1b is gone. It extracted `keyEnv` by grepping the heredoc, so removing the heredoc would have left `facts_key_env` empty and `bad()` called: a check failing for the wrong reason. scripts/verify is now BYTE-IDENTICAL to origin/main, so exactly this PR's addition was removed and none of the pre-existing script was touched. Worth recording why that check was weak independent of the coupling: it asserted "the kit declares this env var itself" by counting occurrences of the value across the whole spec.yaml and requiring >= 2. The only two mentions were a HEADER COMMENT and the heredoc itself, so a check the PR called "security-load-bearing" passed entirely on the strength of one comment line. It also used `grep -c`, which counts matching LINES, not occurrences (verified: `AAA AAA AAA` on one line gives 1, not 3), so its own message was wrong whenever two mentions shared a line. WHAT SURVIVES, and why it is sound: scripts/normalize-catalog.mjs (a HOST-side authoring tool -- one file in, one file out, no network, no credential, matching the containment contract landed on ADR-0004 in #423), its 8 tests, the source fixture, and the vendored snapshot. TWO ADDITIONS beyond the strict rescope, both closing gaps found while doing it: 1. A drift gate for the shipped snapshot (2 tests). Nothing asserted that model-catalog-snapshot.json matched normalizeCatalog(providers/usai/catalog.json), so it was a second hand-maintained copy that could silently diverge from its stated source. It is a deliberate 5-of-21 SUBSET, so the gate allows a subset but requires every kept entry to be byte-equal, and asserts BOTH sides are non-empty first -- an emptied snapshot would otherwise make the comparison pass vacuously. Degraded paths exercised: a mutated cost fails with "has drifted from the normalizer output"; an emptied models[] fails with "nothing to compare, so this gate cannot pass". 2. The snapshot's comment no longer frames it as something a consumer should trust. It ships to /home/agent, which the agent OWNS -- forging it needs no escalation at all, so it was the same invitation as the removed facts file with WEAKER ownership. It is now documented as not authoritative, and a second test asserts the shape stays metadata-only (no host/baseUrl/modelsUrl/keyEnv/apiKey at either envelope or model level), so a routing field cannot be added later without a test failing. Verified: node --test tests/normalize-catalog.test.mjs -> 10 pass, 0 fail (was 8) make test-kits -> 80 pass, 0 fail validate-kits.py --root . --strict -> All 7 acq-kits valid python3 scripts/validate_repo.py --root . -> exit 0 python3 scripts/scan_unsafe_shell.py . -> 0 errors, 0 warnings markdownlint-cli2 -> 0 issues git diff --check -> clean git diff origin/main -- scripts/verify -> empty (byte-identical) NOT in this PR, deliberately: the host-side facts mechanism (needs the upstream `:ro` mount first) and reconciling `acq-neutral-model-catalog/v1` with the already-shipped `usai-model-catalog/v1` in integrations/providers/usai/ -- two catalog schemas coexist, tracked at #435. Decision support: nexus-agents 7-role consensus panel, unanimous 7/7 for this rescope over four alternatives (including converting to the unmerged `:ro` mount, and adding a lockfile to defend a concurrency property sudo defeats anyway). Panel caveat recorded honestly: all 7 seats ran on one model, so independence is weaker than the role count suggests. The agent-owned-snapshot and snapshot-drift gaps were raised by the contrarian seat, not found by me. AI assistance disclosed: prepared by an AI agent (OpenCode) on operator instruction. Human review and merge still required.
…rovider-discovery-local
Addresses mogul's 3 unresolved inline comments from the 2026-10-04 review: 1. Fix the broken Mermaid diagram. The parse error was a literal semicolon in a sequence-message label (`credential; timeout + size cap...`) — Mermaid's sequence-diagram grammar treats `;` as a statement separator inside message text, not literal punctuation. Verified both the broken and fixed form directly against @mermaid-js/mermaid-cli (not just visually): the original text reproduces the exact parse error from the review comment; replacing the semicolon with a comma renders clean. 2. Answer "is the containment contract ever enforced, or just trusted?" with a real answer, not more assertion. Dispatched a research subagent against the actual acq.backends/*.sh codebase (quickstart repo) to check, property by property, whether any of the four containment properties (no network, no credential in env, bounded time, bounded output) has an existing reusable enforcement primitive. Confirmed definitively: none do. What exists are two narrow, hand-copied timeout(1) wrappers that FAIL OPEN (run unbounded) when timeout is absent from the guest, and a sibling repo's JS-level fetch()-body size cap that bounds an HTTP response, not a subprocess's stdout. Added this finding directly to the ADR's Negative/risks section, consulted a security-expert review on the wording (verdict: explicit written acknowledgment of an unenforced trust boundary is the correct, honest thing to put in a design record, not an invitation to exploit it, provided it also states the resulting trust decision and a fail-closed requirement — both added), and added a corresponding "must not decide unilaterally" entry so shipping Layer 2 with the contract merely documented, not enforced, requires the same human/CODEOWNERS review as relaxing it outright. 3. Re-verified all cross-repo links before merge, as asked. Checked every GSA-TTS/agentic-coding-* PR/issue reference in the document against its live current state: #504, #435, #415 all still open and pending as cited. Quickstart issue #506 is now CLOSED (fixed by quickstart#512) — updated its one citation (References section) from a forward-looking "tracking... the upstream fix this ADR depends on" to a past-tense "closed, fixed by #512," consistent with the durable-reference convention already used elsewhere in this document for settled dependencies. Also rebased onto main (7 commits: the markdownlint-cli migration, 2 CVE overrides, the opencode kit, 2 releases, 2 dependency bumps) — none touch this file; this resolves the Markdown Lint CI failure that was purely a result of the branch predating main's markdownlint-cli2 CVE fix. Verification: - node .github/linters/run-markdownlint.mjs: clean, 191 files. - Both Mermaid diagrams verified directly against mermaid-cli (not just read): the trust-boundary flow diagram renders without error. - python3 scripts/validate_repo.py: all validators pass. Co-authored-by: OpenCode Agent <agent@gsa.gov>
|
Addressed all 3 open threads from the 2026-10-04 review, in `f68d604`: 1. Broken Mermaid diagram (line ~448). Root cause: a literal semicolon inside a sequence-message label (`...credential; timeout + size cap...`) — Mermaid's sequence-diagram grammar treats `;` as a statement separator in message text, not literal punctuation. Verified this precisely rather than guessing: reproduced the exact parse error from your review comment against `@mermaid-js/mermaid-cli` using the original text, then confirmed the comma-separated fix renders clean. Both diagrams in the doc now verified to actually render, not just read correctly. 2. "Is the containment contract ever enforced, or just trusted?" Dispatched a research subagent against the real `acq.backends/*.sh` codebase to answer this with facts instead of more assertion. Confirmed definitively: none of the four properties (no network, no credential in env, bounded time, bounded output) has an existing reusable enforcement primitive today. What exists: two narrow, hand-copied `timeout(1)` wrappers elsewhere in acq that fail open (silently run unbounded) if `timeout` is absent from the guest, and a sibling repo's JS-level `fetch()`-body size cap that bounds an HTTP response, not a subprocess's stdout/stderr. Added this finding directly into the ADR's Negative/risks section, ran it past a security-expert review on the framing (verdict: writing down "this is currently unenforced" is the correct, honest thing to put in a design record — the risk would be implying enforcement exists, not disclosing that it doesn't), and the revision now states explicitly: until an orchestrator-controlled mechanism enforces all four, a normalizer must be treated as trusted arbitrary code, not as code already contained by this ADR; a required mechanism being unavailable on a guest must refuse execution and fall back to snapshot, never silently run unenforced. Also added a new "must not decide unilaterally" entry so shipping Layer 2 with the contract merely documented (not enforced) needs the same CODEOWNERS review as relaxing it outright — closes the gap where that call could otherwise be made by omission. 3. Pre-merge link re-check. Checked every cross-repo PR/issue citation against its live state. #504, #435, #415 all still open/pending exactly as cited — no change needed. Quickstart #506 is now closed (fixed by quickstart#512) — updated that one citation from forward-looking "the upstream fix this ADR depends on" to past-tense "closed, fixed by #512," matching the durable-reference convention already used elsewhere in the doc. Also rebased onto main (picks up the markdownlint-cli migration + CVE fixes — none of the 7 commits touch this file; this is what fixes the Markdown Lint CI failure, which was purely staleness, not content). CI 7/7 green. `python3 scripts/validate_repo.py`: all pass. |
mogul
left a comment
mogul
left a comment
There was a problem hiding this comment.
Please address my finding on the sequence diagram... This will be a critical enabler for kit-author understanding!
It's great that we're now being explicit that there's not currently any enforcement mechanism, but... Could there be? For example bubblewrap? At the very least we should have an issue dedicated to figuring that out on the -quickstart side.
Addresses the review finding that the sequence diagram's activations were missing or wrong, and that it was unclear what triggers the bounded fetch and why a filesystem appeared to invoke the normalizer. Both observations were correct, and each was a real defect rather than a rendering artifact: - Nothing invoked the orchestrator. The only arrow into its lifeline went to the read-only mount, so the bounded fetch was a self-message with no inbound trigger. The fetch now follows an explicit `ACQ->>+ORCH: run startup phase`, which is what actually starts it. - A read-only mount cannot originate a message. `RO->>ORCH: run normalizer` drew a passive filesystem as the actor. The normalizer is now its own participant, invoked BY the orchestrator from the mount, which matches the prose: the helper fetches, then invokes the normalizer as a pure file-to-file transform. Also adds what the diagram omitted and the review asked for: - `create time` and `sandbox startup` phase notes, so the ordering is legible without reading the surrounding prose. Kit order is stated on the startup note, since Layers 2-3 depend on it. - Activation bars on the orchestrator and the normalizer, so the nesting of fetch, transform and schema validation is visible. - An `alt` branch for the snapshot fallback. The failure path was described only in prose, which left the diagram implying a single happy path through code whose contract is "failure is a fallback, not a pass". - The agent's failed tamper write moved below an `at any later point` note. It previously sat between provisioning and startup, implying an ordering that was never intended. Two semicolons inside message text are replaced with commas. A semicolon terminates a statement in mermaid's sequence grammar, which is what produced the earlier parse failure; the same mistake would have broken this diagram in the same way. Verified by rendering, not by inspection: the embedded block extracted from the committed file renders to a 37507-byte SVG via mermaid-cli, with every participant and the `alt` branch present and no error graphic. A known-bad control fails hard with no output, so the pass is a real render. Separately, records the pending provider-credential-operations work in References, noting two overlaps a reconciler will need: its diagnostic endpoint and this ADR's `modelsUrl` are the same URL on the same artifact, and the credential environment variable is now recorded in three places. Verified: make validate clean, 407 tests pass, markdownlint clean (192 files). Co-authored-by: OpenCode Agent <william.zujkowski@gsa.gov>
|
Pushed The sequence diagram (your critical-enabler finding)Both of your observations were real defects, not rendering artifacts. Nothing invoked the orchestrator — the only arrow into its lifeline went to the read-only mount, so the bounded fetch was a self-message with no trigger. And a read-only mount cannot originate a message, so Added the init/start events you asked for, plus two things their absence was hiding: the snapshot fallback existed only in prose (now an Verified by rendering rather than inspection: 37507-byte SVG from the committed file, all participants and the Passwordless sudo — you were right, and it is now load-bearingYour original squick was Enforcement — could there be? Yes, and I have not filed it yet
Agreed on both counts, and the issue does not exist — I searched the quickstart tracker and found nothing covering normalizer containment, bubblewrap, or per-subprocess isolation. I did not want to file it against your idea without checking the shape of the problem first, so here is what I measured; tell me if you want me to file it as-is.
So
Worth noting the containment target is narrow, which makes this tractable: the normalizer is a pure file-to-file transform with no network and no credential, so there is no legitimate functionality to preserve when you take those away. LinksRe-checked all five cross-repo references — every "pending" marker is still accurate ( Added your two new issues to References, with a note on two overlaps a reconciler will hit: Verified: AI-assisted (OpenCode). Advisory — requires human review. |
|
The 🤖 is trying to bring this specific idea into scope and flesh it out here just because it was mentioned, and while I'm human-hind-brain-flattered that it finds good things in that direction, it really shouldn't be doing that here. Let's make sure we file a followup issue where all this context belongs! That issue shouldn't be about bubblewrap specifically, but rather about what we're after: An enforceable contract, where bubblewrap is just one idea for getting there.
|
## Context Adds **ADR-0035 (Host-Authoritative Sandbox Configuration)**. This is a docs-only PR (the ADR itself); the acq-side implementation is tracked in #503. acq deliberately gives the in-sandbox agent **passwordless sudo** (the sandbox is the security boundary, per ADR-0005/0011). A consequence not previously stated as a principle: **inside the guest, root-ownership is not a trust boundary against the agent** — a prompt-injected agent can `sudo` to root and rewrite any guest file, including markers under the root-owned `/var/lib/acq/`. acq currently reads four content-bearing `/var/lib/acq` markers back and acts on them to configure the sandbox (`agent`, `workspace`, `ssh-auth-sock`, `kit-env`), and re-runs guest-resident kit startup code across restarts. Both are guest-tamperable under sudo. This surfaced concretely in review of GSA-TTS/agentic-coding-patterns#423 (neutral model-provider discovery), whose provider-facts channel had originally claimed a root-owned guest path was "not agent-writable." ## What this ADR records - **Principle:** acq must not rely on guest-generated or guest-tamperable state to configure the sandbox. Trusted inputs (data *and* code) are host-resident + mounted read-only, or host-held + injected at exec time — never read back from the guest and treated as authoritative. - **Mechanism 1:** per-sandbox host config dir mounted read-only into the guest. - **Mechanism 2:** trusted startup execution invoked from the read-only mount (three options considered; option 1 selected). - **Migration:** stage the existing `/var/lib/acq` markers off guest-writable paths (tracked in #503). - Two Mermaid sequence diagrams (GitHub/Host/Guest boxes) contrasting the rejected guest-write path with the chosen host-materialized read-only path. ## Verification - `markdownlint-cli2` (repo config, v0.22.1): 0 issues. - Both Mermaid diagrams parse cleanly (mermaid v11). - Referenced ADRs (0005, 0011, 0017, 0021, 0023, 0027, 0029) all exist. ## Rollback Docs-only. Revert the single commit / delete `docs/adr/0035-host-authoritative-sandbox-config.md`. No code paths change. ## Security impact Documentation of a security-hardening direction; no runtime change in this PR. The implementing change (#503) narrows what acq trusts; it does not alter the agent's in-sandbox privileges. --- AI-assisted (OpenCode). See the commit's `Co-authored-by` trailer.
## Summary Makes `integrations/isolation/acq-kits/examples/` a container of copy-and-rename kit templates: `validate-kits.py` checks each `examples/<name>/` like a kit but keeps it out of the `kits.yaml` registry. ADR 0005 records why scope-layer kits ship as templates (hybrid/v1 has no parameters) and where they live (placement B). No template ships here; the first one, `team-kit`, and the scope-layers pattern doc follow in a stacked PR. **Reviewer ask: is placement (B), `acq-kits/examples/<name>/` exempt from the registry, the right home?** The alternative (A) is `acq-kits/<name>/` with a `kits.yaml` entry marked TEMPLATE and no validator change; ADR 0005 lists both. Switching to (A) means dropping this PR and moving the template up one level in the next. ## Eligibility & Accountability - [x] I am a **federal employee or contractor** (PRs merge with `@GSA-TTS/agentic-coding-team` code-owner approval; eligibility is attested here, not authenticated). *Anyone may open issues without this.* - [x] **I understand this change and can explain it in my own words** — I am the author of record and accountable for it. - [x] By opening this PR, I **attest I have the right to release this work** under the repository's [CC0-1.0](../LICENSE) dedication. ## AI Assistance Disclosure - [x] AI tools were used in this contribution. - [x] I verified all AI-generated content: no fabricated/hallucinated APIs, dependencies, citations, or placeholder content; any AI-suggested dependency was confirmed to exist. - [x] Any inherited/reused third-party material retains its original license and is identified (not represented as public domain). ## Type of Change - [x] Infrastructure/tooling change - [x] Documentation update ## Motivation Every team adopting `acq` builds the same team scope layer. A template CI keeps schema-valid is more useful than prose alone, but a template is not a kit `acq` applies, so it must not sit in the registry of deployable kits. This PR adds the home and the gate; the content lands separately so the placement question can be answered on its own. What lands: - `acq-kits/validate-kits.py`: skips `examples/` in the kit walk; validates each `examples/<name>/` with the same per-kit checks (schema, `files[].source` resolution, env names, README) under an `examples/` prefix; leaves templates out of the `kits.yaml` cross-check, so a template listed there fails ("registry lists unknown kit"). The walk skips only named tool droppings (`__pycache__`, `.pytest_cache`, `.ruff_cache`, `.mypy_cache`); any other directory, dot-named or `_`-prefixed, is validated, because acq can fetch any directory through a `dir=` ref. Two tree-level checks: no two kits or templates may declare the same `name:` (a copied directory that kept the original name is the likeliest copy-and-rename slip, and the registry cross-check could not see it), and a tree with no kits or templates fails instead of passing with nothing validated. - `scripts/tests/test_validate_kits_examples.py`: 22 tests on generated fixture trees, including duplicate names (kit/kit, template/template, kit/template), empty or missing trees, and dot-named kits and templates; none depend on a real template. - `integrations/isolation/docs/decisions/0005-kit-templates.md`: templates vs a parameterized kit vs doc only, and placement A vs B. Numbered 0005 because #416 and #423 claim 0003 and 0004; happy to renumber. - `acq-kits/README.md`: a Templates section, separate from the Available kits table. ## Testing - [x] Ran `make validate` locally (all checks pass) - [x] Ran `make test` (if applicable): 429 passed; `scripts/tests/test_validate_kits_*.py`: 95 passed - [x] Tested with at least one real scenario: the validator on this repo's real tree with no template present (`All 10 acq-kits valid.`); the stacked PR exercises it with one (`All 10 acq-kits + 1 template(s) valid.`) - [x] Examples are concrete and realistic - [x] Commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) format Also run: `validate-kits.py --strict`, `ruff check scripts/`, `ruff format --check scripts/`, `make generate-check`. ## Safety Checklist - [x] No secrets, API keys, tokens, or passwords included - [x] No PII (names, emails, SSNs, addresses) - [x] No CUI (Controlled Unclassified Information) - [x] No internal URLs, hostnames, or system paths - [x] No customer data or proprietary information - [x] Pattern defines `prohibited_content` in output contract (if applicable): not applicable, no SKILL.md pattern ## Quality Checklist - [x] Plain language used (Grade 10 or below preferred) - [x] Examples included - [x] Prerequisites clearly stated ## Validation Results ``` make validate ✓ All validators passed ``` ## Related Issues Relates to #421 (placement option B) Relates to #416 and #423 (ADR numbering; `validate-kits.py` and `acq-kits/README.md` overlap with #416) ## Reviewer Notes **Reviewers should verify:** - [ ] Placement (B) under `acq-kits/examples/` is the preferred option - [ ] The validator keeps templates out of the registry and still validates them - [ ] `make validate` passes in CI
2 participants
Summary
Records the model-provider discovery design from this session's nexus-agents consensus review work (3 rounds: 7-0 reject → 4-3 split → 5-2 approve), plus the concrete duplication findings that motivate it.
Key findings backing this ADR
acq's secret store dedupes by service-name string only, never by host.usai-providerregisters USAi's credential as serviceusai; the in-flightgoose-serverkit independently registers the same credential, same host, asgoose-usai. Confirmed viaacq.backends/secret-store.sh's_acq_secret_key.Decision
Three layers, no
hybrid/v1schema change required:zscaler-ca-certificate→openchamber/paseopattern), each shipping its own normalizer + vendored fallback snapshot.Both dissenting votes from the final consensus round are incorporated as binding security conditions (env-var-ownership check; SSRF host-matching against the already-effective network allow-list).
Verification
markdownlint-cli2: 0 issues.No implementation in this PR — issues to follow, per this repo's own ADR-then-issues convention.