Repository navigation
Conversation
ed41751 to
cfce305
Compare
|
AI-assisted review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed at head Reading this as a schema change rather than docs+tests: it widens what a kit is allowed to be, letting a kit start host-side Compose services. The ADR is careful and the test file is genuinely thorough (20 cases including degraded paths). Two gaps in the validator, both about the distance between what the ADR asserts in prose and what the code enforces. 1. def _compose_has_privileged_service(compose_doc: object) -> bool:
"""True when any Compose service declares privileged: true."""
return any(
isinstance(service, dict) and service.get("privileged") is True
for service in _compose_services(compose_doc).values()
)I grepped the whole validator for ADR-0003 states the boundary in prose ("a gateway that needs host-level privileges, broad mounts, or Docker socket access is outside this vocabulary"), while the code enforces one keyword. A validator returning 2. Both layers constrain only the entry files in So the schema's "may reference only Compose files shipped inside the kit" is true of the declared list and false of the resulting configuration. Reject One thing I'd like an answer to rather than a change: the trust escalation is documented but not machine-readable. This vocabulary lets a fetched kit start host-side containers, and the control is a sentence — "Remote service-gateway kits must come from trusted kit sources" — with no schema field, no validator check, and nothing telling Also noting, not as a finding: floating image tags are a warning, not an error ( Method: static analysis at head |
|
AI-assisted adversarial review (nexus-agents 1. No schema change in the diff, despite this being described as a schema-level vocabulary (all 5 voters, blocking). The diff touches only 2. Critical: the stated security boundary is documented, not enforced (catfish, severity: critical; confirmed independently by all 5 voters with concrete fixtures). The ADR's premise is that the gateway — not the agent — is the credential/policy enforcement boundary, running outside the agent's reach. The only container-hardening check is
catfish: "checking 3. Path containment is bypassable via Compose's own 4. 5. Host-published ports are accepted as valid endpoint sources (Security, catfish, Scope Steward). 6. ~140 lines of new security-relevant parsing ship with zero tests/fixtures (all 5 voters). No kit in the tree declares 7. ADR numbering collision, confirmed by all 5 voters as requested. Minor/non-blocking, several voters: the closing paren in the The vocabulary concept itself isn't in question (Scope Steward explicitly separated "is this schema idea worth having" — yes — from "is this specific implementation's enforcement real" — no). Given items 2–5 involve genuine container-escape surface in infrastructure every future kit would inherit, I'd treat this one as higher-priority to resolve than a typical implementation-detail review. |
`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.
|
Heads-up, no action needed from you: we've moved out of your way on the ADR number.
You claimed it first and your number isn't cited anywhere outside this PR, so #423 is now ADR 0004 ( Worth noting there's a similar pair still open in quickstart: #507 and #504 both add an My earlier review findings on this PR (the validator's |
|
Re-review requested for PR #416. AI-assisted update by OpenCode Agent; human review is required before merge. Changes since the prior review:
Adversarial review before push:
Verification transcript:
Notes:
|
|
CI/status follow-up for head |
#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.
|
Reviewed the
Context for the scrutiny: a kit declares a Compose file that Blocker 1 —
|
Capture the patterns-side decision for the neutral hybrid/v1 serviceGateways field and its v1 Compose runtime tradeoffs. Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
Add the neutral hybrid/v1 serviceGateways schema and validator checks for kit-local Compose gateways, resolved URL exposure, privileged container rejection, and floating image tag warnings. Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
Reject service-gateway Compose escape primitives, file indirection, host-published ports, and unsafe local file references. Clarify that v1 source trust is human-reviewed rather than schema-enforced, and add validator regression coverage. Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
Reject Compose truthy and interpolated privileged values, bare interpolation in high-risk fields, and device cgroup or supplementary group grants. Co-authored-by: OpenCode Agent <bret.mogilefsky@gsa.gov>
7c1e147 to
48eda7d
Compare
|
Re-review requested for the updated PR branch. Thank you for the corrected, measured review. The reported blockers were valid. This update rebases the branch onto current
An independent read-only adversarial review was run before this push. It identified the missing interpolated- Verification: Commit: AI-assisted update by OpenCode Agent. Human review remains required before merge. Please re-review the updated head. |
|
AI-assisted re-review (OpenCode), advisory — needs a human to confirm before it drives a change. Reviewed head Both blockers and both should-fixes are closed. I re-ran my original bypasses against the new code rather than reading the diff. One new finding, same class as blocker 1. Blocker 1 — truthy-string
|
wz-gsa
left a comment
There was a problem hiding this comment.
Both blockers closed. I re-ran my original reproductions against this head, and separately settled the one residual question I had about the truthy set.
Blocker 1 — string-form privileged now caught
_is_compose_true (:247) replaces the is True identity check. Run against the real function at this head:
privileged=True -> True privileged='1' -> False
privileged='true' -> True privileged=1 -> False
privileged='True' -> True privileged='false' -> False
privileged='TRUE' -> True privileged=False -> False
privileged='yes' -> True privileged=None -> False
privileged='on' -> True
The '1' / 1 question — settled, and your set is exactly right
My open worry was that privileged: 1 and privileged: "1" return False. I could not settle it from the compose-spec schema, which declares privileged as {"type": ["boolean", "string"]} with no enum. So I asked the real binary (piped on stdin, nothing written to disk):
$ docker compose version
Docker Compose version v5.5.1
privileged: 1 -> validating -: services.a.privileged must be a boolean or string
privileged: "1" -> invalid boolean: 1
privileged: "0" -> invalid boolean: 0
privileged: "true" -> privileged: true
privileged: "TRUE" -> privileged: true
privileged: "yes" -> privileged: true
privileged: "on" -> privileged: true
privileged: "y" -> privileged: true
privileged: "n" -> false
privileged: "off" -> false
privileged: "false" -> false
So 1 and "1" are rejected by Compose itself — the file never loads, and there is nothing for the validator to miss. Confirmed upstream in compose-go (loader/interpolate.go:133-147), whose toBoolean accepts exactly true/false/y/yes/on/n/no/off and errors on anything else.
_COMPOSE_TRUE = {"true", "yes", "on", "y"} is therefore precisely Compose's own truthy set, not an approximation. No residual gap. Worth a short comment on that frozenset recording where the set comes from, so a future reader does not "helpfully" add "1" and widen it past what Compose accepts — but that is a nit, not a finding.
Blocker 2 — bare $VAR interpolation now caught
_has_compose_interpolation (:231-241) with _COMPOSE_VAR_RE = \$[A-Za-z_][A-Za-z0-9_]*:
'$HOME:/hostfs' -> True <- my original bypass, now caught
'${HOME}:/hostfs' -> True
'$DOCKER_SOCK' -> True
'$$HOME:/hostfs' -> False <- correct: $$ is a literal $ in Compose
'/safe:/safe' -> False
The $$ handling is the part I would most likely have got wrong myself, and it is right: value.replace("$$", "") before matching, so an escaped dollar is not mistaken for interpolation.
Also the right verdict shape — interpolation is reported as declares privileged with Compose interpolation; validation cannot prove it safe (:285) rather than as a privilege grant. That distinguishes "this is dangerous" from "I cannot tell", which is the distinction that matters for a gate.
Verdict: approving
Both blockers verified closed against the actual code, and the one question I left open resolves in the implementation's favour.
What I did not verify, so the approval is not read as broader than it is: I did not re-run my earlier live-daemon tests at this head (the earlier round proved HostConfig.Privileged=true and CapEff 000001ffffffffff for the string form, and that $HOME:/hostfs mounted a real home directory). Those established the consequence; this round establishes the detection, which is what changed. I also have not exercised include:/extends: resolution, which I raised earlier as a separate should-fix — if that is still open it is not blocking this approval.
AI-assisted review (OpenCode), approved on the maintainer's authority with their explicit go-ahead for this PR. Advisory — a human owns the merge.
## 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
wz-gsa
left a comment
There was a problem hiding this comment.
Correcting the formal review state: the head is unchanged at 48eda7d and the service-hook bypass from my 2026-10-08 review remains unresolved. / hook objects have their own boolean-or-string field in the upstream Compose schema, but the validator and tests do not inspect them. A gateway can therefore pass validation with a privileged lifecycle hook. Please apply the existing / checks to hook entries before merge. The later approval under my account came from a concurrent review that confirmed the original four fixes but did not include this new finding.
Reject privileged and interpolated privileged settings in Compose post_start and pre_stop hooks for service gateways. Co-authored-by: OpenCode Agent <user@gsa.gov>
|
Re-review requested for the updated PR branch. This update resolves the remaining blocking review: service lifecycle hooks can independently declare Added regression coverage for both hook types and both direct-true and Verification transcript: The Make targets were executed with the local Commit: AI-assisted update by OpenCode Agent. Human review remains required before merge. Please re-review the updated head. |
Summary
Add the patterns-side neutral hybrid/v1
serviceGatewayskit vocabulary for kit-declared,acq-managed service gateways.This PR includes:
serviceGateways[], v1 kit-local Compose runtime files, requiredruntime.compose.service, sandbox/agent exposure throughexpose.env, and optionalinterface.portonly when inferable from the named Compose service.expose.envvalues mapped tourl.Tracking: GSA-TTS/agentic-coding-quickstart#472
ADR
The ADR is
integrations/isolation/docs/decisions/0003-service-gateways-vocabulary.md.It captures the core decisions:
serviceGateways.runtime.compose.serviceto identify the gateway service in multi-service Compose files.expose.env.Verification
Commands run:
PATH="$PWD/.venv/bin:$PATH" make validate-kitsResult: passed. All 7 acq-kits valid.
Result: passed. 94 tests passed.
Result: passed.
PATH="$PWD/.venv/bin:$PATH" make validateResult: passed. Existing advisory warnings were reported for documented PII/CUI terms and network-tier deep-subdomain review signals.
PATH="$PWD/.venv/bin:$PATH" make generate-checkResult: passed.
INDEX.yamlandCATALOG.mdare up to date.Result: passed. 422 tests passed.
Result: passed.
AI Assistance
This PR was prepared with AI assistance from OpenCode Agent. A human reviewer remains responsible for review and merge.