docs(dark-factory): Autonomous Agent Coding Pattern design - #32
Open
elamaran11 wants to merge 270 commits into
Open
docs(dark-factory): Autonomous Agent Coding Pattern design#32elamaran11 wants to merge 270 commits into
elamaran11 wants to merge 270 commits into
Conversation
…un Kata) Comment out agent_sandbox on dev. Spoke clusters run EKS Auto Mode + Bottlerocket, which cannot host Kata micro-VMs (no nested-virt/kvm control). Chart is kept; enabling needs a kata-capable nodepool strategy for Auto Mode first. Verified: YAML parses.
…run Kata) Same as dev: comment out agent_sandbox until EKS Auto Mode + Kata integration is designed. Chart retained. Verified: YAML parses.
…options Record why agent-sandbox is disabled on spokes: EKS Auto Mode + Bottlerocket cannot host Kata (no nested-virt/kvm control, no kata-deploy). List 3 paths to evaluate: self-managed kata nodepool, Bedrock AgentCore/Fargate microVM, or gVisor. Top item to resolve before enabling Flow A live.
Record the spike outcome: self-managed nested-virt MNG coexists with EKS Auto Mode and /dev/kvm works via CpuOptions.NestedVirtualization (aws-cli >=2.35). Decision: MNG (not a 2nd Karpenter); AgentCore/gVisor rejected with reasons. Two implementation lessons: AL2023 nodeadm MIME userData (don't clobber bootstrap) + delete MNG-first teardown ordering. Replaces the old blocker note.
Depend on the upstream kata-deploy chart (v3.32.0) to install the Kata runtime + containerd handlers on kata MNG nodes. Gated by kataDeploy.enabled so the chart stays inert on clusters without a kata-capable nodepool. Verified: YAML parses.
Mirror openclaw's kata-deploy config: target the static kata-enabled label (not katacontainers.io/kata-runtime, which deadlocks), tolerate kata + runtime-not-ready taints, install qemu+clh shims, disable kata-deploy's own RuntimeClasses (our chart owns them). Default kataDeploy.enabled=false so it's inert without a kata nodepool. Verified: YAML parses.
The correct AL2023 MIME userData for the nested-virt kata MNG: a boot script that modprobes kvm_intel + persists it (so /dev/kvm exists before Ready), then a nodeadm NodeConfig that joins the cluster with the kata-enabled label + kata=true taint. Encodes spike lesson #1 (don't clobber nodeadm bootstrap).
Declarative nested-virt (c8i/m8i) Managed Node Group for spoke-dev alongside Auto Mode: minSize 0 (scale-to-zero), kata-enabled label + kata=true taint, AL2023, scoped node IAM. Coexistence validated by the spike. Verified: YAML parses.
…late Terraform variant for teams on the appmod/openclaw IaC path: launch template with cpu_options.nested_virtualization=enabled (the flag eksctl can't set) + MIME nodeadm userData (modprobe kvm_intel + join), MNG scale-to-zero, kata=true taint, kata labels. Verified: braces balanced.
…pass) Expand one-line variable blocks to valid multi-line HCL; terraform fmt applied. Verified with real terraform: 'terraform validate → Success! The configuration is valid.'
…ate ArgoCD app) An unbuilt OCI subchart dependency blocks the whole chart from rendering (helm won't template with a missing dep) and is fragile under ArgoCD. Remove the dependency; kata-deploy will be a separate gated ArgoCD Application pulling the upstream OCI chart directly. Verified: chart renders standalone again + lint clean.
Add kata-deploy to the addon catalog as its own app (OCI chart from ghcr.io/kata-containers, sync-wave 1) gated by enable_agent_sandbox_kata, so only clusters with a kata-capable nested-virt MNG install the runtime. Mirrors openclaw config: target kata-enabled label, tolerate kata + runtime-not-ready taints, qemu+clh shims, our chart owns RuntimeClasses. Verified: YAML parses.
…te app) kata-deploy moved to its own gated ArgoCD app, so remove the subchart values block from the chart; leave a pointer note. Verified: values parse + chart still renders 14 manifests.
…equence) Document the kata MNG provisioning options (eksctl/Terraform/nodeadm userData), the 4-step per-cluster enablement sequence (provision MNG → enable_agent_sandbox_kata → enable_agent_sandbox → warm pool), cost (scale-to-zero), and teardown ordering.
…317) The Bifrost OTEL plugin requires settings under 'config:' key with 'collector_url' (not 'endpoint') and 'protocol: grpc' on port 4317. Previous config was ignored by Bifrost because: - Settings were at wrong nesting level (missing config:) - Used 'endpoint' instead of 'collector_url' - Used HTTP/4318 instead of gRPC/4317 This fix enables Bifrost to export LLM call spans to the OTel Collector, completing the unified trace tree: Agent spans + Bifrost LLM spans merged under the same W3C traceparent.
The upstream chart requires 'version: 1' and supports 'trace_type: genai_extension' for Gen AI semantic spans.
The default/addons/bifrost/values.yaml plugins block overrides the chart's values.yaml. The otel plugin must be in both places for Helm merge to include it in the rendered ConfigMap.
…ely redundant) Live testing found the upstream operator ships sandboxwarmpools.extensions. agents.x-k8s.io (spec: replicas + sandboxTemplateRef) — a first-class warm-pool primitive. Our custom pool-manager CronJob is likely redundant; prefer a SandboxWarmPool CR. To reconcile after the live test.
Always wrap with opentelemetry-instrument as the container command. This enables httpx auto-instrumentation which injects W3C traceparent headers into outbound HTTP calls (agent → Bifrost). Without this, Bifrost creates a separate trace instead of attaching as child spans. Centralized mode: traceparent propagated → Bifrost spans appear as children in the same Langfuse trace tree. Decentralized mode: ADOT distro activates → CW GenAI export.
…r (lesson #3) Live test caught it: with a custom ImageId, nodeadm does NOT auto-discover the cluster API endpoint/CA and fails 'Apiserver endpoint is missing in cluster configuration' — node boots with /dev/kvm but never joins. Add apiServerEndpoint, certificateAuthority, and cidr to the NodeConfig in both the reference MIME userData and the Terraform (via aws_eks_cluster data source). Verified: terraform validate passes.
KubeVela drops container command/args overrides. Moving the opentelemetry-instrument wrapper into the Dockerfile CMD ensures it's always active regardless of how the CUE renders. This enables httpx auto-instrumentation which propagates W3C traceparent headers to Bifrost, correlating agent + Bifrost spans into a single trace tree.
…uto Mode (lesson #4) Live test: Auto Mode clusters have no vpc-cni addon (built-in networking is Auto-Mode-nodes-only), so a self-managed kata MNG node joins but stays NotReady with 'cni plugin not initialized'. Fix: install the vpc-cni EKS addon; aws-node tolerates all taints and schedules onto the kata node. Verified: node went Ready + /dev/kvm present after installing vpc-cni.
… handles it Since Dockerfile CMD is now opentelemetry-instrument, the auto-instrumentation configures the tracer provider from OTEL_EXPORTER_OTLP_ENDPOINT env var. The manual StrandsTelemetry().setup_otlp_exporter() call created a conflicting second provider that swallowed spans. Only manual init needed: direct-to-Langfuse mode (LANGFUSE_BASE_URL set) where custom auth headers must be constructed.
- Drop name/namespace/serviceAccount params; use context.name/context.namespace everywhere (Rollout/Services/SA/container/AGENT_NAME/route). Component name is now the single identity across k8s, gateway (sub), AWS pod identity, and A2A. - Agent owns a dedicated ServiceAccount (context.name) — the anchor the gateway-identity/aws-service-identity traits attach to (traits can only read context.name, not parameter.name). - Examples: simple (gateway-identity trait), agentcore-memory (gateway-identity + aws-service-identity accessFor, replacing dp-service-account wiring). BREAKING: OAM Applications must drop properties.name/namespace; the component name + app namespace drive identity.
…oy open item Two live tests on spoke-dev proved: MNG+Auto-Mode coexistence, /dev/kvm, node joins Ready (with nodeadm endpoint/CA fix + vpc-cni addon). kata-deploy itself crashloops on Auto Mode due to the experimental nydus snapshotter restarting containerd + dropping CNI mid-run (lesson #5, fix: disable nydus). Also noted the top-level-vs-nested kata-deploy values gotcha for direct helm installs.
opentelemetry-instrument exports metrics and logs by default to the same OTLP endpoint. The OTel Collector only has a traces pipeline, so metrics export returns 404. Setting OTEL_METRICS_EXPORTER=none and OTEL_LOGS_EXPORTER=none for centralized mode stops the errors. Decentralized mode keeps metrics/logs enabled (CloudWatch handles them).
… traits Drop properties.name/namespace/serviceAccount; attach gateway-identity where the agent uses the gateway/MCP; decentralized-observability attaches the observability identity trait. Aligns all examples with the refactored agent component.
…r + options Live test confirmed: kata-deploy restarts containerd → drops the aws-node CNI on the same node → its own API call fails → crashloop before install completes. Disabling nydus snapshotter did not help (the restart is the trigger). Everything up to runtime install is proven. Options: bake Kata into a custom AMI (Packer), use openclaw's self-managed Karpenter model, or a no-restart containerd config.
opentelemetry-instrument provides httpx auto-instrumentation (traceparent propagation) but does NOT create Strands-specific spans (agent invocations, tool calls). StrandsTelemetry().setup_otlp_exporter() is needed to activate the Strands tracer which uses the same OTLP exporter configured by env vars. Both coexist: opentelemetry-instrument handles HTTP context propagation, StrandsTelemetry handles Strands SDK span creation.
…ernal-name Live Observe-only (managementPolicies:[Observe]) dry-run adoption test against the hub cluster surfaced two bugs in template 17 that helm-template render-checking could not catch (both would silently fail under ArgoCD): 1. kind: Nodegroup -> kind: NodeGroup. The upbound provider-aws-eks CRD's real kind is NodeGroup (capital G); 'Nodegroup' has no REST mapping so the CR would never apply. 2. external-name '<cluster>/<nodegroup>' -> bare '<nodegroup>'. The provider prepends clusterName itself; 'hub/kata-sandbox' produced an external ID of 'hub:hub/kata-sandbox' -> 400 InvalidParameterException from DescribeNodegroup. Bare 'kata-sandbox' adopts cleanly. Verified live: with the corrected kind + external-name, an Observe-only NodeGroup imported the existing hub kata-sandbox node group (SYNCED=True READY=True, status ACTIVE, LT lt-0e204ea3e305e2e1f v2, min0/desired1/max3, kata taint, correct subnets/role/labels) with ZERO mutation and no node churn. LaunchTemplate (external-name = LT id) and Addon (external-name '<cluster>:<addon>') formats were also probed and adopt correctly as-is.
shapirov103
requested changes
Jul 28, 2026
shapirov103
left a comment
shapirov103
left a comment
Contributor
There was a problem hiding this comment.
Review — PR #32 (Dark Factory)
The design is strong. The holdout gate, the second judge model, the VPC-CNI ClusterIP find, and the Kata-on-Auto-Mode spike are all good work.
But the doc says the guardrails are enforced by design. In the code, they rest on trust instead. Three of those gaps block merge.
There is also cleanup to do: a leaked account ID, a committed session log, and demo config in the chart defaults.
1. Remove the account ID (11 spots)
940019131157 sits in the chart defaults. So it ships as the default for anyone who installs this chart. And the repo is public.
| File | Lines |
|---|---|
gitops/addons/charts/dark-factory/values.yaml |
93, 125, 132, 133, 134, 214, 262 |
gitops/addons/charts/agent-sandbox/values.yaml |
160 |
gitops/addons/clusters/hub/addons/agent-sandbox/values.yaml |
29 |
docs/dark-factory/AGENT-INSTALL.md |
17 |
docs/dark-factory/RESTORE-STATE.md |
47 |
What is in each spot:
dark-factory/values.yaml:93—evalImage:ECR URIdark-factory/values.yaml:125— comment naming the account and regiondark-factory/values.yaml:132—irsaRoleArn: arn:aws:iam::940019131157:role/df-securityagent-irsadark-factory/values.yaml:133—serviceRoleArn: arn:aws:iam::940019131157:role/service-role/df-securityagent-service-roledark-factory/values.yaml:134—diffBucket: dark-factory-secagent-940019131157-us-west-2dark-factory/values.yaml:214—reviewImage:ECR URIdark-factory/values.yaml:262—image:deploy-test ECR URIagent-sandbox/values.yaml:160— coderimage:ECR URIclusters/hub/addons/agent-sandbox/values.yaml:29—nodeRoleArnforhub-kata-node-roleAGENT-INSTALL.md:17— example accountRESTORE-STATE.md:47— account plus agent space IDs
Fix: leave chart defaults blank. Put the real values in the per-cluster overlay. This PR already does that for the kata nodepool, so the pattern is there. In docs, use <ACCOUNT_ID>.
iam/securityagent.tf is fine. It takes the account as a variable.
2. Remove the session log
docs/dark-factory/RESTORE-STATE.md is a work-in-progress checkpoint, not docs. It holds:
- a Claude Code session ID,
ade840e8-b310-4ac7-a173-98e4fbbc5098 - Security Agent space
as-0fa95663…and DevOps space65fe3629… - the account number
- a run-by-run log of the session
One commit says this file was deleted. It is still here at branch HEAD. Please drop it.
Commit messages also name internal-only things. The files were cleaned up later, but the messages stay in public history:
- ticket
V2290161680 HeimdallonboardingReleaseManagement*CDKconstantsMidway
Squash or reword these before merge.
3. Take the demo config out of the defaults
The demo setup ships as the shipped config:
dark-factory/values.yaml:311— repo iselamaran11/dark-factory-sandboxdark-factory/values.yaml:315—webhookUrl: https://idp.elamaras.people.aws.dev, a personal dev domainholdout/elamaran11-dark-factory-sandbox/— the only scenario set in the chart. Four toysubtract()cases.- ClusterIP
172.20.181.17:8080is hardcoded indark-factory/values.yaml:33and:88,agent-sandbox/values.yaml:178, and as a code default inholdout/evaluate.js:31. The comment names the real fix (Kata VMdnsConfig) and defers it to P2. agent-sandbox/values.yaml:157–159— comment says the image is a busybox placeholder. Line 160 already sets the real image.agent-sandbox/nodepool/— the.tf, eksctl, and.mimefiles are dead now. Crossplane templates 16/17/18 replaced them. That chart has no.helmignore, so they get packed into the Helm artifact.
This is more than tidiness. The holdout scenarios only cover the author's sandbox repo. On any other repo the gate just passes. No scenarios file means exit 0 and "skipping gate (advisory)". Zero matching scenarios means ratio = 1, which also passes. So on a real repo the headline quality gate reports success without checking anything.
4. Code findings
Blocking
C1 — the coder VM gets the orchestrator's full GitHub token.
templates/05-externalsecret-github.yaml copies one Secrets Manager value (property: token) into three namespaces. One of them is agent-sandbox-system, as gh-token. And agent-sandbox/values.yaml:156 mounts that into the coder VM at /etc/secrets.
The doc says the coder gets a "short-TTL GitHub token (contents:write only)" in §10 and §4. The real token needs much more. It needs admin:repo_hook, because active: true registers the webhook. It needs merge rights, because merge.js calls PUT /pulls/N/merge.
So the coder can merge to main, push to main, and comment as the factory. "The agent never self-merges" holds only because the agent's own code chooses not to. Nothing stops it.
Its prompt is issue text that anyone can write. GitHub egress has to stay open. That is the lethal trifecta again — the very thing §11.1 says this design breaks. And it lands in the one part built to avoid it.
Fix: give the coder a GitHub App token, scoped to contents:write on the target repo, minted per run. Give the orchestrator a separate credential.
C2 — the holdout gate runs coder code outside the Kata VM.
20-workflowtemplate-df-run.yaml:346 runs npm install --no-audit --no-fund. There is no --ignore-scripts, so the coder's preinstall and postinstall hooks run. Then scenarios.json calls require(process.env.REPO+'/app/index.js'), which runs the coder's module code in the same process.
That step uses the workflow service account from line 28. It carries the IRSA AWS role, a mounted K8s token, and GH_TOKEN. Nothing sets automountServiceAccountToken: false anywhere in the template.
So model-written code runs in a trusted hub pod with AWS creds, K8s API access, and a GitHub write token. That code is exactly what the Kata VM, the three network layers, and the tainted node group exist to contain. The check step walks around all of it.
Fix: add --ignore-scripts. Set automountServiceAccountToken: false on every step that touches coder output. Run the evaluator in a sandbox. Give IRSA to the security-agent step only.
C3 — merge.js passes when checks are missing.
const notGreen = REQUIRED.filter((c) => by[c] && by[c] !== "success");
const missing = REQUIRED.filter((c) => !by[c]);
if (notGreen.length) { ...exit 1 }
if (missing.length) console.log(`note: checks not present (treated as skipped): ...`);A missing check gets logged, then the merge goes ahead. Only a check that exists and failed will stop it. §9 says merge.js requires every dark-factory/* status to be green. It does not. A crashed step, a timeout, or a Bifrost outage turns the gate into a pass.
A second issue makes this worse. merge.js reads only pr.head.sha. But status.js reads all PR commits on purpose, because statuses drift off head after a push. So after a df-iterate force-push, the status board can look green while merge.js sees no checks at all — and merges.
Fix: treat missing checks as a failure. Also set this in branch protection, with required checks and dismiss-stale-approvals. The merge gate should not rest on a script that uses a token the coder also holds. See C1.
High, but not blocking
- Test namespaces have no Pod Security labels.
deploy-test.shcreates the namespace with onlydark-factory.io/*labels. Then it applies manifests the coder wrote. The workflow account can create pods and deployments in any namespace. A privileged pod with ahostPathmount gets root on the node. Addpod-security.kubernetes.io/enforce=restrictedat create time, plus aResourceQuota. - The judge reads coder-controlled text.
evaluate.jsdropsDIFF.slice(0, 12000)into the prompt and tells the judge to "default to PASS". So attacker text reaches the one check meant to catch cheating. Strip comments, tell the model the diff is data and not instructions, and reject diffs that touch test or CI files. clusterip-egress-firewallfails open. It runsapk add iptablesat start, unpinned, with output hidden. If that fails, the pod crashloops. While it does, the ClusterIP hole stays open — the same hole the doc says layer 3 is there to close. Bakeiptablesinto the image. Hold coder scheduling until the DaemonSet is ready.
Doc drift
- The PR says "doc-only, no runtime code". The branch has 92 files, about 25.8k added lines, and 247 commits. It includes a privileged
NET_ADMINDaemonSet and cluster-scoped RBAC. - §6.2 says the DevOps review runs as a GitHub App check. The DAG comment in
20-workflowtemplate-df-run.yamlstill says it runs as a Claude Code plugin inside the coder step.
Bottom line
C1 and C2 matter most. Together they mean the isolation story, the best part of this PR, does not hold yet. The rest is cleanup.
Both charts pass helm lint. helm template renders 36 resources with no errors. Packaging is fine.
Contributor
Author
|
@shapirov103 Nice review and summary of findings. @punkwalker Please confirm what are all the items already fixed in local version. Please check in those changes. |
punkwalker
added a commit
that referenced
this pull request
Jul 30, 2026
Addresses the PR #32 review plus the credential split the review's C1 asked for. GitHub credentials — three scoped identities instead of one shared token (C1). The coder VM previously received the orchestrator's full-power token, so the credential that can merge to main lived inside the one component running model-written code driven by attacker-controllable issue text. Now: events hub / argo-events metadata+contents read, webhooks write orchestrator hub / argo pull requests + commit statuses write coder sandbox / agent-sandbox contents write ONLY Provisioned by ExternalSecrets from AWS Secrets Manager, keyed <cluster>/dark-factory/github/* so each cluster's ESO IRSA can be scoped to its own prefix — the sandbox cluster cannot read the hub's orchestrator token. The orchestrator secret lands in argo.namespace, NOT the release namespace: Workflow pods resolve secretKeyRef in their own namespace, and the WorkflowTemplates are created in argo. Documented in docs/dark-factory §10a (SM keys, GitHub permissions, create-secret commands, verification, failure symptom). merge.js: absent status is no longer a pass (C3). `missing` was computed and only logged, so a crashed step, a timeout, a Bifrost outage, or statuses left on a pre-force-push commit all merged as though verified — the gate failed OPEN. Both `missing` and `notGreen` are now fatal. The required list can't be hardcoded, because each gate is individually switchable (holdout.enabled, review.enabled — which posts security AND devops via a dynamic context — deployTest.enabled), so a fixed list would demand statuses a disabled gate never posts and block every merge. It's derived from the enabled flags via dark-factory.requiredChecks and passed in as REQUIRED_CHECKS. This also picks up dark-factory/deploy-test, which was posted but never required. holdout: npm install --ignore-scripts (C2, partial). That step runs on the trusted hub, not in the Kata VM, over a coder-authored tree — without the flag a preinstall/postinstall hook executes in a pod holding GH_TOKEN and a mounted SA token, walking around the VM boundary. automountServiceAccountToken:false is NOT included: without a paired executor.serviceAccountName it stops the executor writing workflowtaskresults and fails every holdout/review step, and that isn't verifiable here. Cleanup: the four image defaults carrying ECR account 940019131157 are now empty and wrapped in Helm `required`, with the concrete URIs moved to per-cluster overlays — so a missing image fails at render with a named error instead of emitting `image:` empty and failing at pod creation. NOTE those URIs are a FOREIGN account (this deployment is 259732043995) and are almost certainly not pullable; they are preserved as-is so behaviour is unchanged, and flagged inline. Also: last hardcoded Bifrost ClusterIP replaced with service DNS; dead kata-mng {.tf,eksctl,.mime} removed (nodepool/README kept — kata-nodepool/README links it); .helmignore added to both charts, deliberately NOT excluding dark-factory's holdout/ and review/, which .Files.Get reads to build the evaluate/review/merge ConfigMaps (ignoring them renders those ConfigMaps EMPTY with no render error). Also included: kata-nodepool intra-app sync waves (EC2NodeClass 0 before NodePool 1, so nodeClassRef resolves on a first pass); trigger.argoEvents.active made configurable via hasKey (| default true would collapse an explicit false); layer-5 consumer files (fleet/hub overlay.repoURL + clusters/hub/karpenter manageAddons) which stay INERT until appmod-blueprints PR #798 lands; and the root Taskfile AWS_CONFIG_FILE hotfix that resolves AWS_PROFILE via IMDS on EC2. Verified: all three charts lint; each renders as ArgoCD composes it (chart + overlay); the required guards fail loudly without an overlay and pass with one; Files.Get ConfigMaps still carry content after .helmignore; overlay paths match what the live Applications actually read.
v0.2.4 carries the new entrypoint.js: emits artifacts/description.md so the PR body gets a '📝 Changes' section (reviewer request). Bumped on the deployed branch so the hub agent-sandbox app picks it up on sync. Warm-pool sandboxes must be recycled to pull the new image.
Two fixes for the 'security review waits ~15 min while the PR already shows a result' problem: 1. Early-exit: the code-review job's status field flips to COMPLETED long AFTER the AWS Security Agent App bot has already posted its findings on the PR. So in addition to polling status, each iteration now probes list-findings; the moment it returns a well-formed result (findingsSummaries present) we proceed — no waiting for the status flip. Reuses those findings for the report (no re-fetch). 2. Graceful timeout: this step is advisory (the App bot posts the authoritative result regardless), so exceeding the poll timeout now posts a neutral 'pending' status + a pointer to the bot comment, instead of a misleading red 'error'. This also fixes the stale sticky board: the sticky-status step depends on security-agent (runs once, after it), so a fast security step means the board is rewritten with accurate rows within seconds instead of ~15 min later. sh -n passes.
Make the reviewer set deterministic — the DevOps + Security agents now appear as reviewers on EVERY df PR, not just when their own auto-review happens to land on the current SHA (root cause of the #54-has-them / #56-doesn't inconsistency: an agent review binds to a commit SHA, and a re-run/force-push orphans it). - status.js: for each REVIEWER_BOTS slug, POST /pulls/N/requested_reviewers (idempotent; failures logged, never fatal). Runs on every sticky-status update, after the PR exists. - df-run: pass REVIEWER_BOTS to the sticky-status step (gated on .reviewerBots). - values: reviewerBots = [aws-security-agent[bot], aws-devops-agent-us-east-1[bot]] — the [bot] App slug is what the requested_reviewers API accepts (verified live; bare name is rejected 422 'not a collaborator', [bot] returns 200). The agents then post their COMMENTED review (LGTM / findings) on the requested SHA. Verified: node --check status.js; df-run renders REVIEWER_BOTS + valid YAML.
…viewer-request) The requested_reviewers approach was ineffective: GitHub App bots (aws-security-agent[bot], aws-devops-agent-us-east-1[bot]) CANNOT be added as requested reviewers — the API silently no-ops (returns [], zero timeline events, verified 3x). They review autonomously + inconsistently (#54 got formal reviews; #56/#58 only got issue comments), which is inside the agent services, not something we control via that API. Replace it with a CONSISTENT signal: status.js posts ONE consolidated verdict review (event=COMMENT, as the workflow identity) summarizing Build/Holdout/Security/ DevOps once verification is terminal — idempotent via a hidden marker, gated by POST_VERDICT_REVIEW. LGTM line when all green. The agents' own checks (dark-factory/security, aws-devops-agent/...) remain the source of truth; COMMENT (not APPROVE) so the human still owns merge. Reverted the no-op REVIEWER_BOTS code. Verified: node --check; df-run renders POST_VERDICT_REVIEW, valid YAML, no stale REVIEWER_BOTS.
…ed on async DevOps) The DevOps Agent App reviews ASYNCHRONOUSLY — its check is usually still PENDING when sticky-status runs (once, at workflow end). The prior 'overall != pending' gate therefore never fired (sticky-status doesn't re-run), so the verdict review never posted. Gate instead on the steps the workflow controls (implementation + security resolved); render a still-pending DevOps as 'in progress' in the review body. This makes the consolidated verdict review post reliably every run.
…e/multiply/fibonacci/power The holdout scenario set only had subtract-* scenarios, all gated on /index\.js/.test(diff) — so ANY change to app/index.js (e.g. a factorial PR) triggered all four subtract scenarios. The judge, asked whether a FACTORIAL diff genuinely implements SUBTRACT, flaked to NO on one, giving 3/4 (75%) and a spurious holdout FAILURE on correct factorial code. Fix: key each scenario's appliesWhen on its OWN function name (/factorial/.test(diff), /gcd/.test(diff), ...) so a PR is graded only by the scenarios for the function it changes; unrelated scenarios SKIP, not fail. Added genuine basic+anti-stub scenarios for the functions the repo now exports (multiply, fibonacci, power, gcd, factorial, isPrime); kept add-regression as the one file-scoped baseline. Documented the scoping model in the rubric so new functions get a matching scoped block.
The PR carried 3-5 overlapping restatements of the same verdict (a DevOps relay comment + a Security relay comment + the consolidated review + the agents' own App reviews), and — worse — a correctness bug: with the Security step advisory (blockLevel: none), real findings were listed in a comment while the check stayed green, so the consolidated verdict printed 'no findings / LGTM' right next to a findings table. Mixed signals. Fixes: - securityAgent.blockLevel: none -> medium. Findings at/above medium fail the dark-factory/security check, which fails the run and gates the merge, so the consolidated verdict can never say cleared/LGTM while real findings exist. - Remove the DevOps relay comment (dark-factory:devops-agent) from devops-gate. - Remove the Security relay comment (dark-factory:security-agent) from security-agent.sh (keeps the commit STATUS it reads from). - Result: ONE pipeline artifact — the consolidated verdict review — mirroring the source-of-truth checks (Build/Holdout/Security/DevOps), plus the agents' own App reviews. No contradictory LGTM.
… lies Root cause of the false 'no findings / LGTM': the pipeline read the HEADLESS dark-factory/security commit status, but the real aws-security-agent[bot] runs its OWN scan on the PR and caught findings the headless copy missed (e.g. a wildcard-ARN DynamoDB IAM policy on #67). The two disagreed and we trusted the wrong one. status.js: - Reads the actual agent BOT reviews (aws-security-agent[bot], aws-devops-agent-*[bot]) + their inline review comments as authoritative. Parses 'N finding(s)' vs 'no issues identified'; headless dark-factory/* is a fallback ONLY when a bot didn't post. - overall now derives from the resolved bot verdicts → a bot with findings makes overall=failure. The verdict prints '❌ Changes requested — do NOT merge … address the findings', never 'LGTM', when findings exist. - Posts the consolidated review as REQUEST_CHANGES (falls back to COMMENT) when an agent flagged issues, so the PR visibly shows changes-requested. merge.js: - Added a source-of-truth gate: refuse to merge if the Security or DevOps agent BOT posted findings, even if the headless commit status is green — closes the exact gap that let #67 read as mergeable. Human still owns the merge approval; agents' own reviews remain the source of truth.
Removes the confusing, contradictory review surface and replaces it with a
single coherent flow driven by the REAL AWS agents.
WHAT WAS WRONG:
- Two security scans: our headless dark-factory/security (security-agent.sh) ran
a SECOND scan that disagreed with the real aws-security-agent[bot] — reported
'no findings' while the bot flagged real issues (wildcard-ARN IAM) -> false LGTM.
- Stale PR body: the coder wrote per-step states ('Holdout: running…', 'Security:
runs after DevOps clears…') at PR-open that were only rewritten at the very end,
so mid-run they lied.
- Consolidation fired before the async security bot finished -> premature verdict.
THE CLEAN FLOW:
- security-agent step no longer scans; it WAITS FOR the real aws-security-agent[bot]
review (new scripts/security-wait.js) and mirrors its verdict into
dark-factory/security (findings -> failure -> merge blocked). Deleted the
redundant security-agent.sh. ONE security signal: the agent itself.
- Runs in parallel with devops-gate (both wait on their real bots), no longer
gated behind DevOps clearing.
- The consolidated step depends on security-agent, so the pipeline now
naturally waits for the real bot before posting the verdict.
- Coder PR body is now a NEUTRAL 'verification in progress' pointer to the
consolidated review — no stale per-step guesses.
- Consolidated review surfaces the DevOps Agent's report link (target_url) +
inline-comment counts.
Result: build/holdout/deploy-test (ours) + the two real AWS agent bots = one
consolidated verdict. Findings -> changes-requested + merge blocked; clean -> LGTM.
Two holdout bugs surfaced on a sum-only PR that wrongly failed 6/8: 1. The diff was built with 'git diff origin/BASE...HEAD' on a branch clone with no shared merge-base → the WHOLE index.js read as added, so every pre-existing function name appeared in the diff and every scenario's appliesWhen matched (subtract/multiply/fibonacci/power/gcd all graded on a sum PR). Switched to the GitHub compare API (/pulls/N/files patch hunks) — same authoritative-diff fix already used by detect-deployable — so appliesWhen keys on the ACTUAL change. 2. Holdout is advisory (holdout.blocking=false) but was feeding the consolidated 'overall' verdict, so a flaky judge on an unrelated scenario flipped a clean PR to 'changes requested'. status.js now excludes advisory holdout from the merge verdict (HOLDOUT_BLOCKING env from holdout.blocking); the row still shows, but only real Security/DevOps agent findings or a build break block the merge.
…block scalar) The multi-line python -c in 14cfa96 sat at column 0 inside the bash source: block scalar and broke YAML parsing (helm: 'could not find expected :' line 308). Collapsed to a one-liner that stays within the block scalar. helm template renders.
…s cleared When overall is green but the advisory holdout row is ❌, the verdict no longer claims 'all cleared with no findings' — it says 'Cleared to merge; holdout below threshold but ADVISORY, review before merging.' Merge decision unchanged.
Makes the fix -> re-gate -> verdict loop actually CLOSE, and auto-drives it. ROUND-AWARENESS (needed even for human-driven iteration): - Verdict review was a ONE-SHOT (skipped if any prior verdict existed) so it never moved ❌->✅ on a fix round. Now tagged with the head SHA (<!-- df-verdict-sha:<sha> -->): posts a FRESH verdict per commit, idempotent within a SHA. The PR visibly closes the loop. - status.js + security-wait.js now only trust a bot review/inline comment whose commit_id == current head SHA (else a prior round's '1 finding' leaks into the new verdict). security-wait also time-guards issue-comment acks vs the head commit. AUTO-FIX loop (review.autoFixFindings, default true): - On a ❌ verdict from the real agents, status.js collects the Security review summary + both agents' inline findings and submits a bounded df-run revision (df-run-<issue>-fix<N>, iterate-note = the findings) via the in-cluster k8s API (dark-factory-workflow SA already has workflows:create). Coder auto-revises the df/issue-N branch — no human paraphrasing. Bounded by iterate.maxIterations via the df-iterations/<n> label; 409 dedup per round. Human only approves at the end. - Kata substrate: fix round claims a NEW coder (resumes the branch). Lambda (Flow D): resumes the suspended MicroVM (keeps context) — the payoff of suspend/resume. Render + node --check clean.
… stale-HEAD race) Root cause found live on df-run-77-fix1: on a fix round the branch's OLD commit already carries dark-factory/implementation=success (from the first pass), so await-coder accepted it IMMEDIATELY and the verify fan (holdout/security/devops) ran against the STALE pre-fix HEAD — the coder's fixed commit landed 45s LATER. Result: security-wait mirrored the OLD '1 finding' and the loop couldn't close. Fix: await-coder records the branch's start SHA and, when iterate-note is set (fix round), REQUIRES a different head SHA (a genuinely new commit) before considering the coder done — so the verify steps run against the FIXED commit. First-pass runs (no iterate-note) are unchanged. Also outputs head-sha for downstream pinning. Render clean.
…r submit)
The sticky-status env references {{workflow.parameters.trigger-label}} but the
df-run WorkflowTemplate never DECLARED trigger-label in arguments.parameters, and
the sensor's embedded submit spec doesn't pass it — so every sensor-fired run
failed: 'failed to resolve {{workflow.parameters.trigger-label}}'. Declared it
with default 'dark-factory' so all references resolve regardless of submitter.
Verified: server dry-run submit WITHOUT trigger-label now succeeds.
…n broke the fix round df-run-84-fix1 failed: 'claim-sandbox.resource.manifest must be a valid yaml'. The auto-fed agent findings are multi-line markdown (quotes/braces/colons) injected raw into the SandboxClaim env YAML → invalid manifest → the fix workflow never started. Fix: pass the note as BASE64 via a new iterate-note-b64 param (single-line, safe for arbitrary text); the coder decodes DF_ITERATE_NOTE_B64 (falls back to plain DF_ITERATE_NOTE). status.js (auto-fix) + iterate.js (human comment) both base64 now. await-coder's fix-round detection also checks the b64 param. Coder change requires a v0.2.5 image build (entrypoint.js iterateNote() decode).
…-gate + security-agent) Requirement: each fix round must decide on COMPLETE input — wait for BOTH DevOps AND Security before computing the ❌/✅ verdict + the auto-fix decision, and a clear verdict must mean both agents actually cleared. The status (sticky-status) step depended on holdout+security+deploy-test but NOT devops-gate, so the verdict could compute before DevOps was terminal. Added devops-gate to its dependencies.
v0.2.5 adds DF_ITERATE_NOTE_B64 decode (iterateNote()) so the coder receives auto-fed agent findings safely (base64) on a fix round — required for the findings->fix loop. amd64 (Kata) + arm64 (Lambda) both built + pushed to ECR.
punkwalker
added a commit
that referenced
this pull request
Jul 31, 2026
Addresses the PR #32 review plus the credential split the review's C1 asked for. GitHub credentials — three scoped identities instead of one shared token (C1). The coder VM previously received the orchestrator's full-power token, so the credential that can merge to main lived inside the one component running model-written code driven by attacker-controllable issue text. Now: events hub / argo-events metadata+contents read, webhooks write orchestrator hub / argo pull requests + commit statuses write coder sandbox / agent-sandbox contents write ONLY Provisioned by ExternalSecrets from AWS Secrets Manager, keyed <cluster>/dark-factory/github/* so each cluster's ESO IRSA can be scoped to its own prefix — the sandbox cluster cannot read the hub's orchestrator token. The orchestrator secret lands in argo.namespace, NOT the release namespace: Workflow pods resolve secretKeyRef in their own namespace, and the WorkflowTemplates are created in argo. Documented in docs/dark-factory §10a (SM keys, GitHub permissions, create-secret commands, verification, failure symptom). merge.js: absent status is no longer a pass (C3). `missing` was computed and only logged, so a crashed step, a timeout, a Bifrost outage, or statuses left on a pre-force-push commit all merged as though verified — the gate failed OPEN. Both `missing` and `notGreen` are now fatal. The required list can't be hardcoded, because each gate is individually switchable (holdout.enabled, review.enabled — which posts security AND devops via a dynamic context — deployTest.enabled), so a fixed list would demand statuses a disabled gate never posts and block every merge. It's derived from the enabled flags via dark-factory.requiredChecks and passed in as REQUIRED_CHECKS. This also picks up dark-factory/deploy-test, which was posted but never required. holdout: npm install --ignore-scripts (C2, partial). That step runs on the trusted hub, not in the Kata VM, over a coder-authored tree — without the flag a preinstall/postinstall hook executes in a pod holding GH_TOKEN and a mounted SA token, walking around the VM boundary. automountServiceAccountToken:false is NOT included: without a paired executor.serviceAccountName it stops the executor writing workflowtaskresults and fails every holdout/review step, and that isn't verifiable here. Cleanup: the four image defaults carrying ECR account 940019131157 are now empty and wrapped in Helm `required`, with the concrete URIs moved to per-cluster overlays — so a missing image fails at render with a named error instead of emitting `image:` empty and failing at pod creation. NOTE those URIs are a FOREIGN account (this deployment is 259732043995) and are almost certainly not pullable; they are preserved as-is so behaviour is unchanged, and flagged inline. Also: last hardcoded Bifrost ClusterIP replaced with service DNS; dead kata-mng {.tf,eksctl,.mime} removed (nodepool/README kept — kata-nodepool/README links it); .helmignore added to both charts, deliberately NOT excluding dark-factory's holdout/ and review/, which .Files.Get reads to build the evaluate/review/merge ConfigMaps (ignoring them renders those ConfigMaps EMPTY with no render error). Also included: kata-nodepool intra-app sync waves (EC2NodeClass 0 before NodePool 1, so nodeClassRef resolves on a first pass); trigger.argoEvents.active made configurable via hasKey (| default true would collapse an explicit false); layer-5 consumer files (fleet/hub overlay.repoURL + clusters/hub/karpenter manageAddons) which stay INERT until appmod-blueprints PR #798 lands; and the root Taskfile AWS_CONFIG_FILE hotfix that resolves AWS_PROFILE via IMDS on EC2. Verified: all three charts lint; each renders as ArgoCD composes it (chart + overlay); the required guards fail loudly without an overlay and pass with one; Files.Get ConfigMaps still carry content after .helmignore; overlay paths match what the live Applications actually read.
punkwalker
added a commit
that referenced
this pull request
Jul 31, 2026
Addresses the PR #32 review plus the credential split the review's C1 asked for. GitHub credentials — three scoped identities instead of one shared token (C1). The coder VM previously received the orchestrator's full-power token, so the credential that can merge to main lived inside the one component running model-written code driven by attacker-controllable issue text. Now: events hub / argo-events metadata+contents read, webhooks write orchestrator hub / argo pull requests + commit statuses write coder sandbox / agent-sandbox contents write ONLY Provisioned by ExternalSecrets from AWS Secrets Manager, keyed <cluster>/dark-factory/github/* so each cluster's ESO IRSA can be scoped to its own prefix — the sandbox cluster cannot read the hub's orchestrator token. The orchestrator secret lands in argo.namespace, NOT the release namespace: Workflow pods resolve secretKeyRef in their own namespace, and the WorkflowTemplates are created in argo. Documented in docs/dark-factory §10a (SM keys, GitHub permissions, create-secret commands, verification, failure symptom). merge.js: absent status is no longer a pass (C3). `missing` was computed and only logged, so a crashed step, a timeout, a Bifrost outage, or statuses left on a pre-force-push commit all merged as though verified — the gate failed OPEN. Both `missing` and `notGreen` are now fatal. The required list can't be hardcoded, because each gate is individually switchable (holdout.enabled, review.enabled — which posts security AND devops via a dynamic context — deployTest.enabled), so a fixed list would demand statuses a disabled gate never posts and block every merge. It's derived from the enabled flags via dark-factory.requiredChecks and passed in as REQUIRED_CHECKS. This also picks up dark-factory/deploy-test, which was posted but never required. holdout: npm install --ignore-scripts (C2, partial). That step runs on the trusted hub, not in the Kata VM, over a coder-authored tree — without the flag a preinstall/postinstall hook executes in a pod holding GH_TOKEN and a mounted SA token, walking around the VM boundary. automountServiceAccountToken:false is NOT included: without a paired executor.serviceAccountName it stops the executor writing workflowtaskresults and fails every holdout/review step, and that isn't verifiable here. Cleanup: the four image defaults carrying ECR account 940019131157 are now empty and wrapped in Helm `required`, with the concrete URIs moved to per-cluster overlays — so a missing image fails at render with a named error instead of emitting `image:` empty and failing at pod creation. NOTE those URIs are a FOREIGN account (this deployment is 259732043995) and are almost certainly not pullable; they are preserved as-is so behaviour is unchanged, and flagged inline. Also: last hardcoded Bifrost ClusterIP replaced with service DNS; dead kata-mng {.tf,eksctl,.mime} removed (nodepool/README kept — kata-nodepool/README links it); .helmignore added to both charts, deliberately NOT excluding dark-factory's holdout/ and review/, which .Files.Get reads to build the evaluate/review/merge ConfigMaps (ignoring them renders those ConfigMaps EMPTY with no render error). Also included: kata-nodepool intra-app sync waves (EC2NodeClass 0 before NodePool 1, so nodeClassRef resolves on a first pass); trigger.argoEvents.active made configurable via hasKey (| default true would collapse an explicit false); layer-5 consumer files (fleet/hub overlay.repoURL + clusters/hub/karpenter manageAddons) which stay INERT until appmod-blueprints PR #798 lands; and the root Taskfile AWS_CONFIG_FILE hotfix that resolves AWS_PROFILE via IMDS on EC2. Verified: all three charts lint; each renders as ArgoCD composes it (chart + overlay); the required guards fail loudly without an overlay and pass with one; Files.Get ConfigMaps still carry content after .helmignore; overlay paths match what the live Applications actually read.
…IS commit Observed on df-run-86-fix1: security-wait reported '1 finding' while the bot was STILL reviewing the fix commit (a 'reviewing…' ack posted AFTER our status). Root cause: it resolved on bare INLINE comments, which (a) arrive incrementally while the bot analyses and (b) GitHub can re-attribute a prior round's inline comment to the new head SHA — so the count was premature/stale, and the summary findings weren't on the PR yet when we posted. Fix: resolve ONLY on a TERMINAL signal for this commit — a formal REVIEW with a summary body, or an explicit 'No issues identified' comment — and only AFTER this round's 'reviewing…' ack (proves the bot started on THIS commit). Inline comments are used only to COUNT once terminal, never to decide done. status.js now prefers the authoritative dark-factory/security status (set by the waiter) over re-parsing, so they never disagree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Design document (doc-only, no runtime code) for a Dark Factory — Autonomous Agent Coding Pattern on the Open Agent Platform.
A human files a GitHub issue (a spec); AI agents do the rest — implement → build → test → security/devops review → PR → (human approves results) → merge → teardown. Autonomy Level 3.
Contents
docs/dark-factory/README.md— full designdocs/dark-factory/diagrams/flow-a-sandbox-capability.md— Flow A: the permanent Agent Sandbox capability (Kata/CLH micro-VMs +SandboxCRD + warm pool), capability architecture + warm-pool state machinedocs/dark-factory/diagrams/flow-b-dark-factory.md— Flow B: the Dark Factory pipeline, end-to-end flow + detailed sequence + live-status mockDesign highlights
eks-platform-openclaw(Sandbox CRD, session-router lifecycle, LiteLLM→Bedrock),appmod-blueprints(PlatformClusterephemeral EKS),agent-platform-amazon-eks(addon ApplicationSets).Validated against industry practice
Aligned with GitHub Copilot coding agent, OpenAI Codex, Devin, and StrongDM's Software Factory. Explicitly designs against documented anti-patterns: lethal trifecta / prompt injection, reward hacking / test-gaming, LLM-judge self-preference bias, multi-agent over-orchestration, non-converging comment loops, warm-pool idle burn, and rubber-stamp reviews. Sources cited in the README.
Note
Doc-first by design — this PR ships the pattern/design. Implementation follows the phased roadmap (P1–P5) in the README. Mermaid diagrams render natively on GitHub.