feat: add @swamp/kubernetes extension with 15 models and 14 workflows #4
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-k8s"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adopts John Watson's k8s-swamp-extension into this repo as a hand-written Kubernetes operational toolkit. This provides first-class K8s support across pods, deployments, services, namespaces, nodes, RBAC, secrets, configmaps, ingress, PVCs, HPAs, jobs, network policies, and events — plus 14 ready-to-run workflows for namespace debugging, security audits, RBAC analysis, cluster health, and operational diagnostics.
What's included
_lib/helpers.ts) with typed K8s client builder, metadata normalization, and instance name sanitizationChanges from the original repo
The extension was adopted without preserving git history or the original development structure. The following changes were made to align with this repo's conventions and best practices:
extension.yaml→manifest.yaml; addedadditionalFiles,labels,platformsfieldsservice.tscreate/update methods were missingnamespacein their argument schemas — users couldn't override namespace per-callz.any()withz.record(z.string(), z.unknown())in pod/deployment create methodsV1Pod,V1Deployment, etc.) instead of untyped parameters|| {}fallback patterns with optional chaining to fix TypeScript strict-mode errors_default/_fromK8s SDK reserved word mapping in namespace (LimitRange) and netpol models@kubernetes/client-nodeimports to pinnednpm:@kubernetes/client-node@1.0.0for quality tool compatibilitykubernetes-check(lint + fmt) andkubernetes-lockfilejobs to both Forgejo and GitHub CI workflows; wired into review gates and adversarial review triggers; addedkubernetes/to review prompt scopeWhy this is correct
_default,_from) verified producing correct datadeno lint,deno fmt --check, anddeno install --frozenall passmanifest.yamlchangesTest plan
swamp extension quality manifest.yamlpasses (14/14)deno lint extensions/models/passesdeno fmt --check extensions/models/passesdeno install --frozenpassesswamp doctor extensions_frompeers verified)Co-Authored-By: John Watson john@systeminit.com
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
CI Security Review
Critical / High
No critical or high severity findings.
Medium
No medium severity findings.
Low
.forgejo/workflows/ci.yml:89,139,etc. — Matrix values interpolated directly inrun:blocks. Pattern likeif [ "${{ matrix.task }}" = "fmt" ]interpolates Actions expressions directly into shell. While the matrix values are hardcoded in the workflow file ([check, lint, fmt, test], etc.) and therefore not attacker-controlled, the general pattern of direct expression interpolation inrun:blocks is fragile — a future edit adding user-derived matrix values would silently introduce injection. The safe pattern isenv: TASK: ${{ matrix.task }}thenif [ "$TASK" = "fmt" ]. Same applies tomatrix.vault,matrix.datastore, etc. Not exploitable today since all matrix values are author-controlled constants. This observation applies equally to.github/workflows/ci.yml..forgejo/workflows/ci.yml:8-9 — Workflow-level permissions vs job-level. Permissions are set at workflow level (permissions: contents: read) rather than per-job. In this case the effect is positive: all jobs are restricted to read-only, which is the most restrictive default possible. The review jobs that need to post comments use a separateBOT_TOKENvia curl rather than relying onGITHUB_TOKENpermissions. The net security posture is equivalent to job-level scoping since every job needscontents: readand no job needs more fromGITHUB_TOKEN. Not exploitable.Analysis Notes
Prompt Injection (Priority 1): Clean. Neither workflow interpolates attacker-controlled event data (PR body, issue title, comment body, commit messages) into LLM prompts. The Forgejo workflow reads prompt templates from files (
.forgejo/prompts/*.md) and appends only file paths fromgit diff --name-only. The GitHub workflow uses inline prompts with onlygithub.repository(owner/repo format) andgithub.event.pull_request.number(numeric) interpolated. All three prompt templates include security preambles instructing the model to treat PR content as untrusted.Tool Scoping: Tightly scoped in all review jobs. Both workflows restrict Claude to
Read,Grep, and narrowly-scopedBashpatterns (git diff:*,git log:*,teeto a specific file,touchfor a specific file). The GitHub workflow also allowsgh pr review:*,gh pr view:*,gh pr diff:*— all read/review operations, no write operations beyond review submission.Triggers: Both workflows use
pull_request(safe trigger, runs in PR context). Nopull_request_target,issue_comment, orworkflow_dispatchtriggers.Supply Chain: All actions are from trusted publishers (
actions/*,denoland/*,anthropics/*) or SHA-pinned (dorny/paths-filter@fbd0ab8...in the GitHub workflow). The Forgejo workflow downloads the Claude CLI binary viacurlbut verifies it against a hardcoded SHA256 checksum before execution — equivalent to SHA pinning.Secret Handling: Secrets (
ANTHROPIC_API_KEY,BOT_TOKEN,GITHUB_TOKEN) are passed viaenv:blocks to the steps that need them, not leaked in logs or interpolated in shell commands where they could be exposed. The Forgejo workflow's comment-posting step JSON-encodes the review body via Python'sjson.dumps(), preventing payload injection.Auto-merge: Neither workflow auto-merges. The GitHub workflow allows Claude to approve PRs via
gh pr review --approve, but merge is governed by branch protection rules, not the workflow.Verdict
PASS — The CI pipeline changes are well-designed from a security perspective. No prompt injection vectors, tight tool scoping, proper secret handling, verified supply chain, and appropriately scoped permissions.
Adversarial Review
Critical / High
kubernetes/extensions/models/pod.ts:324-338—restartmethod assumes the recreated pod keeps the same name, which is wrong for Deployment-managed podsThe
restartmethod deletes a pod, then polls for a pod with the exact same name to come back asRunning. This only works for StatefulSet pods (which preserve names) and bare pods. For the most common case — pods managed by a Deployment/ReplicaSet — the new pod gets a different generated name. The method will always time out, log a misleading warning ("Pod not yet running after Ns wait"), and return empty handles, even though the pod was successfully restarted.Breaking example: Call
restarton a pod namednginx-deploy-7d4b8f4f9-x2kl9owned by a Deployment. The pod is deleted (side-effecting), a new podnginx-deploy-7d4b8f4f9-abc12is created by the ReplicaSet, but the polling loop at line 327 looks for the old name and never finds it.Suggested fix: After deletion, instead of polling by exact name, list pods matching the deployment's label selector and look for a new Running pod, or simply return immediately after deletion (the delete itself is the restart for controller-managed pods) and document the limitation.
kubernetes/extensions/models/namespace.ts:422-447and542-565—setResourceQuotaandsetLimitRangecatch ALL errors, masking real failuresThe try/catch blocks intend to implement "replace if exists, create if not found." But the catch is bare — it catches every exception, not just 404 (NotFound). If
readNamespacedResourceQuotasucceeds butreplaceNamespacedResourceQuotafails (e.g., invalidhardvalues, permission denied, 409 conflict), the code falls through tocreateNamespacedResourceQuota, which throws "already exists." The user gets a confusing "already exists" error instead of the actual validation/permission error.Breaking example: Call
setResourceQuotawithhard: { "cpu": "invalid" }. The read succeeds, the replace fails with a validation error, the catch swallows it, the create fails with "AlreadyExists", and the real error is lost.Suggested fix: Check the error status code in the catch — only fall through to create on HTTP 404. Re-throw all other errors:
Medium
kubernetes/extensions/models/ingress.ts:155-175and231-247,netpol.ts:193-227—namespaceis referenced in execute but missing from the Zod arguments schemaThe
createandupdatemethods iningress.tsand thecreatemethod innetpol.tsreferenceargs.namespace(e.g., line 178:const ns = args.namespace ?? context.globalArgs.namespace), butnamespaceis not declared in theirargumentsZod schema. Zod strips unknown keys, soargs.namespaceis alwaysundefinedand the namespace override silently has no effect. Compare withlist,get, anddeletein the same files which DO includenamespacein their schemas.This is inconsistent — users can override namespace on list/get/delete but not create/update.
Suggested fix: Add
namespace: z.string().optional()to the arguments schema for all affected methods.kubernetes/extensions/models/node.ts:92— implicitanytype on parameter violates CLAUDE.md strict mode ruleThe
typeparameter has no type annotation, making it implicitlyany. CLAUDE.md mandates "TypeScript strict mode, noanytypes in hand-written code." This would be caught bydeno checkbut CI only runslintandfmtfor kubernetes (see next finding).Suggested fix:
const findAddress = (type: string) => {.forgejo/workflows/ci.yml:267and.github/workflows/ci.yml:273— CI skipsdeno check(type-checking) for the kubernetes extensionBoth CI configs define
task: [lint, fmt]for the kubernetes-check job. Every other extension in CI uses[check, lint, fmt, test]or at least[check, lint, fmt]. Omittingcheckmeans TypeScript type errors (like the implicitanyin node.ts:92) are not caught by CI. This undermines type safety for the entire extension.Suggested fix: Add
checkto the matrix:task: [check, lint, fmt]kubernetes/extensions/models/_lib/helpers.ts:7—process.setMaxListeners(0)globally disables listener leak detectionSetting
setMaxListeners(0)removes the limit for the entire Node process, not just the K8s client listeners. If any other code in the same process has a genuine listener leak, the warning that would normally detect it is suppressed. The comment acknowledges this is intentional, but the blast radius is process-wide.Suggested fix: If the K8s client exposes its EventEmitter instances, increase the limit only on those. If not, at minimum set it to a high finite number (e.g., 100) rather than unlimited.
Low
kubernetes/manifest.yaml:4— description says "13 ready-to-run workflows" but the manifest lists 14 workflows (lines 36-51).Trivial docs inconsistency.
kubernetes/extensions/models/_lib/helpers.ts:28-59—buildClientcreates 7 new API client instances on every method callEach model method call invokes
buildClient(), which instantiates KubeConfig plus 7 API client objects. For batch operations (list → iterate → get each), this creates many short-lived client instances. The K8s client-node library may pool HTTP connections internally, but the object churn is unnecessary. A per-model-instance cache keyed on context+kubeconfig would be more efficient.HPA metric correlation by array index (
hpa.ts:105-107) could mismatch if status metrics are reorderedThe code assumes
spec.metrics[i]corresponds tostatus.currentMetrics[i]. The K8s API documentation says the order should correspond, but the code has no validation if the arrays differ in length beyond the shorter-array case. Unlikely in practice.Verdict
FAIL — Two HIGH findings require fixes before merge: (1)
pod.restartsilently fails for Deployment-managed pods (the most common use case), and (2)setResourceQuota/setLimitRangeswallow real errors via overly broad catch blocks, masking the actual failure reason. The missingnamespacein ingress/netpol create schemas is an inconsistency that should also be addressed.d87dd81ff395721d9d77CI Security Review
Critical / High
kubernetes-checkandkubernetes-lockfilemissing frommerge-gateneeds —.forgejo/workflows/ci.yml:974-1002The new
kubernetes-checkandkubernetes-lockfilejobs are added as dependencies ofclaude-review(line 660) andclaude-adversarial-review(line 774), but are not added to themerge-gatejob'sneedsarray (line 974). Every other extension's check and lockfile jobs (vault-*,datastore-*,ssh-*,workflows-*,cve-*, etc.) are listed inmerge-gate.needs, making this an omission.Why this matters: The
merge-gatejob is the single roll-up check that determines whether a PR can merge. It usescontains(needs.*.result, 'failure')to detect failures — but only among its listed dependencies. Sincekubernetes-checkandkubernetes-lockfileare not listed, their failures are invisible to the merge gate.Cascade effect: Because
claude-reviewandclaude-adversarial-reviewboth list the kubernetes jobs in theirneedsand use!failure()as a gate condition, a kubernetes check failure causes those review jobs to be skipped (not failed). The merge-gate sees'skipped'for the review jobs, which does not match'failure', so merge-gate passes. This creates a path where:kubernetes/alongside other codekubernetes-checkfailsclaude-reviewandclaude-adversarial-revieware silently skippedmerge-gatepasses, potentially allowing merge without code reviewFix: Add
kubernetes-checkandkubernetes-lockfileto themerge-gate.needsarray, matching the pattern used by all other extension jobs:Medium
No medium findings.
Low
No low findings.
Notes
The remainder of the changes are security-neutral or positive:
review.md,adversarial.md): Addingkubernetes/to the review scope lists is a safe, mechanical change. Both prompts retain their security preambles about treating PR content as untrusted data.kubernetes-check,kubernetes-lockfile): Follow the exact same pattern as existing extension jobs (ssh-check,ssh-lockfile). They useactions/checkout@v6anddenoland/setup-deno@v2(trusted publishers), inherit the workflow-levelcontents: readpermission, use no secrets, and run onlydeno check/lint/fmtcommands — all appropriate.check_path kubernetes "kubernetes/"addition and output wiring follow the established pattern.kubernetesto thefindpath list is correct.needs.changes.outputs.kubernetes == 'true'to the trigger condition is correct.Read,Grep, specificBashpatterns).pull_request) is unchanged.Verdict
FAIL — The
kubernetes-checkandkubernetes-lockfilejobs must be added to themerge-gate.needsarray. Without this fix, kubernetes check failures silently skip the code review gates and the merge gate still passes.Code Review
Blocking Issues
anytypes in all 15 hand-written model files — CLAUDE.md violationCLAUDE.md is explicit: "no
anytypes in hand-written code (generated code may useany)". Every model file inkubernetes/extensions/models/suppresses the linter with// deno-lint-ignore-file no-explicit-anyat the top, and then usesanypervasively in allexecutemethod signatures:This appears in all 15 hand-written model files (
pod.ts,deployment.ts,secret.ts,service.ts,configmap.ts,ingress.ts,node.ts,rbac.ts,pvc.ts,hpa.ts,job.ts,netpol.ts,namespace.ts,event.ts,pod_summary.ts). Thenode.tsalso addscontext.globalArgs as anycasts.The prior commit (
95721d9d) shows this was deliberately adopted as a "repo convention," but it still violates CLAUDE.md as written. The fix is either to provide typed interfaces forargsandcontextfrom the swamp framework, or update CLAUDE.md to formally document this exception for framework boundary parameters.Suggestions
No tests and no test CI step for the kubernetes extension
The
deno.jsonhas notesttask and thekubernetes-checkCI job only runscheck,lint, andfmt— no test step. All other extensions (vault, datastore, issue-lifecycle, ssh, workflows, cve) have test tasks and CI test matrix entries. CLAUDE.md's testing requirement is scoped to vault/ and datastore/, so this is not a rule violation, but the extension is completely untested. Given that many methods make mutating K8s API calls (create, delete, scale, cordon, taint), at least in-memory mock client tests for the normalization helpers would add value.HPA metric merging by positional array index is fragile (
hpa.ts:106)The HPA status metrics (
status.currentMetrics) are not guaranteed to be in the same order as the spec metrics (spec.metrics). If the controller reorders or adds entries, pairing by index would silently associate the wrong current value with the wrong target. A more robust approach would match bytype+namerather than by index.service.tsandpod.tsomitsanitizeInstanceNameon somewriteResourcecallsIn
service.ts(list, get, getEndpoints, create, update, diagnoseService) andpod.ts(list, get, create), the resource key passed tocontext.writeResourceisnormalized.nameorargs.serviceNamedirectly, without going throughsanitizeInstanceName. Every other model routes names throughsanitizeInstanceName. This inconsistency means a service or pod with..or/in its name (unusual but possible for endpoints) would not be sanitized.Adversarial Review
High
secret.ts:167 and secret.ts:226 -- btoa() throws InvalidCharacterError on non-Latin-1 secret values
The create and update methods use btoa(value as string) to base64-encode user-supplied secret values. btoa() only accepts Latin-1 characters (U+0000-U+00FF). Characters above U+00FF (emoji, CJK, Cyrillic, Arabic) crash with unhandled InvalidCharacterError.
Breaking example: Creating a secret with password containing emoji throws InvalidCharacterError. Also affects certificates and binary data.
Suggested fix: Use encodeBase64 from jsr:@std/encoding/base64 with TextEncoder instead of btoa().
Medium
namespace.ts:824-827 -- Health check treats Succeeded pods as unhealthy
The health method requires podsRunning === podsTotal. Succeeded Job pods count in podsTotal but not podsRunning. Namespaces with completed batch jobs always report healthy: false.
Breaking example: 5 Running + 2 Succeeded pods yields podsRunning=5, podsTotal=7, healthy=false.
Suggested fix: Count Succeeded pods as healthy.
namespace.ts:770-779 -- Named targetPorts cause false-unhealthy reports
parseInt on named ports like http returns NaN. NaN === anything is false. String comparison of 8080 against http also fails. Services with named targetPorts always marked unhealthy.
Breaking example: targetPort http with pod port name http containerPort 8080 reports no listeners.
Suggested fix: Add pc.name === target to port matching.
hpa.ts:106-108 -- Spec/status metrics joined by array index
Assumes status.currentMetrics[i] matches spec.metrics[i]. K8s API does not guarantee ordering. Mismatch pairs wrong current/target values.
Suggested fix: Match by (type, name) tuple instead of index.
Low
pod.ts:387-390 -- Multi-container metrics are comma-joined strings, not totals
deployment.ts:359 -- update targets only first container silently
Verdict
FAIL -- btoa() crash (finding 1) is HIGH. Health check bugs (2, 3) affect common K8s patterns.
CI Security Review
Summary of Changes
This PR adds CI coverage for a new
kubernetes/extension by:kubernetesto the change detection filter in thechangesjobkubernetes-checkandkubernetes-lockfilejobs (check, lint, fmt, lockfile verification)claude-review,claude-adversarial-review, andmerge-gatekubernetes/to thedeps-auditoutdated dependency scankubernetes/to the adversarial review trigger conditionkubernetes/to the directory focus lists in bothreview.mdandadversarial.mdpromptsAll changes follow the exact same patterns as the existing SSH, workflows, CVE, and other extension CI jobs.
Checklist Results
1. Prompt Injection — No new risks. The LLM-invoking jobs (
claude-review,claude-adversarial-review,claude-ci-security-review) are unchanged in structure. No attacker-controlled event fields are interpolated into prompts. Tool scoping remains tight (Read,Grep, specificBashpatterns). Security preambles are present in both prompt files.2. Expression Injection & Script Injection — No new risks. The new
kubernetes-checkjob uses${{ matrix.task }}inrun:blocks, but the matrix values are static ([check, lint, fmt]) — not attacker-controlled. This is the same pattern used by all other*-checkjobs.3. Dangerous Triggers — No change. The workflow triggers on
pull_request(notpull_request_target), which is safe. No new triggers introduced.4. Supply Chain — No new risks. The new jobs use
actions/checkout@v6(GitHub-owned) anddenoland/setup-deno@v2(trusted publisher), both acceptable with tag-only pins per policy. No new third-party actions or remote script execution introduced.5. Permissions — No issues. The new
kubernetes-checkandkubernetes-lockfilejobs inherit the workflow-levelcontents: readpermission, which is the minimum they need. No permission escalation.6. Secret Exposure — No risks. The new kubernetes jobs do not use or reference any secrets. They only run
deno check,deno lint,deno fmt, anddeno install --frozen.7. Auto-merge & Trust Boundaries — No weakening. The
merge-gatejob now requires kubernetes jobs in addition to existing jobs, making the gate strictly more comprehensive. The adversarial reviewif:condition now also triggers on kubernetes changes — this is additive security coverage, not a bypass.Medium
(none)
Low
(none)
Verdict
PASS — All changes are security-neutral, following established CI patterns exactly. The PR adds Kubernetes extension CI coverage using the same job templates, action pins, permission model, and LLM tool scoping as all existing extension jobs. No new attack surface introduced.
Adversarial Review
Critical / High
node.ts:316—context.logger.warn()will crash at runtime when metrics-server is absentThe shared
Loggerinterface in_lib/helpers.ts:24-27defineswarning(), notwarn(). Every other model usescontext.logger.warning(...)(seepod.ts:414,service.ts:452). Butnode.ts:316callscontext.logger.warn(...). If the platform logger implements theLoggerinterface from helpers.ts (which provideswarningbut notwarn), this will throw aTypeError: context.logger.warn is not a functionat runtime.Breaking scenario: Any cluster without metrics-server installed (extremely common — it's an optional add-on). Every call to the
node.getMetricsmethod will crash instead of gracefully returning an empty result.Suggested fix: Change
context.logger.warn(tocontext.logger.warning(on line 316 ofnode.ts, matching the pattern inpod.ts:414.Medium
namespace.ts:738— Inconsistent error suppression ingetResourceCountsPromise.allThe
getResourceCountsmethod runs 7 parallel API calls viaPromise.all. Only thedeploymentscall (line 738) has.catch(() => 0)— the other 6 calls (pods,services,configmaps,secrets,serviceaccounts,pvcs) do not.Breaking scenario: If the caller's service account lacks RBAC permission to list pods (but can list deployments), the deployment count silently returns 0 while the pod count throws, crashing the entire method. The inconsistency suggests either the
.catchwas a leftover from debugging, or all calls should have it.Suggested fix: Either add
.catch(() => 0)to all 7 calls (if partial results are acceptable) or remove it from the deployments call (if the method should fail fast on any RBAC error).namespace.ts:908-911— Health check marks namespaces with completed Jobs as unhealthyThe
healthmethod's healthy boolean requirespodsRunning === podsTotal. Pods inSucceededphase (normal for completed Jobs, one-shot pods, init containers) count towardpodsTotalbut notpodsRunning.Breaking scenario: A namespace with a healthy Deployment and a completed Job:
podsRunning=3, podsTotal=4→healthy: false, even though nothing is wrong.Suggested fix: Either count
Succeededpods as healthy in the running tally, or separate Job-owned pods from the health calculation.pod_summary.ts:148— UnguardedJSON.parsecrashes entire summarize on one bad entryIf any single stored pod resource has corrupted or non-JSON content, this throws and aborts the entire summarize operation. Since this reads data written by other model instances (cross-model aggregation), the input is not fully under this code's control.
Breaking scenario: A single malformed pod resource entry (truncated write, encoding issue, platform bug) kills the summary for all pods.
Suggested fix: Wrap in try/catch, log a warning for the bad entry, and continue with the remaining pods.
Low
All models —
buildClient()creates 7+ fresh API clients per method call, no reuseEvery method invocation calls
buildClient()which instantiates a newKubeConfig, parses it, and creates 7 API clients + a Metrics client. In workflows that call multiple methods in sequence (the 14 workflow YAMLs all chain multiple model calls), this creates dozens of redundant client instances per workflow run.No tests for any Kubernetes model code
The CI job for kubernetes (
ci.yml:267) only runscheck,lint,fmt— notesttask. There are no test files in the PR. The CLAUDE.md testing rules recommend local HTTP servers and conformance helpers. Without tests, the runtime bugs above (e.g.,warnvswarning) would have been caught.Verdict
FAIL — The
context.logger.warn()call innode.ts:316will crash at runtime on any cluster without metrics-server, which is a very common deployment scenario. This is a one-character fix (warn→warning) but it blocks because it's a runtime crash on a common code path.Code Review
Blocking Issues
None.
Suggestions
hpa.ts— spec/status metric merging by array index is unreliable (kubernetes/extensions/models/hpa.ts:127–138)normalizeHpapairs spec metrics and statuscurrentMetricsby position (statusMetrics[i]). Kubernetes does not guarantee thatstatus.currentMetricsentries align withspec.metricsentries in the same order or that they have equal length. If the two arrays differ in length or ordering, the displayedcurrentValueends up associated with the wrong metric target. The safest fix is to match by(type, name)instead of index.pod.ts—execresult discards stderr (kubernetes/extensions/models/pod.ts:499–515)stderris decoded and conditionally logged but never stored in theexecResultresource. TheExecResultSchemahas nostderrfield. Callers that use the resource for debugging can never retrieve stderr output from successful-but-noisy commands. Consider addingstderr: z.string()to the schema and including it in the result object.pod.ts—sanitizeInstanceNamenot applied to exec result key (kubernetes/extensions/models/pod.ts:516–519)Every other
writeResourcecall in the kubernetes models passes the instance name throughsanitizeInstanceName, but the exec result uses the raw user-supplied value:Kubernetes pod names are DNS-safe in practice, but for consistency and defence-in-depth it should be
sanitizeInstanceName(${args.podName}-exec).deno.json—@systeminit/swamp-testingimported but unused (kubernetes/deno.json:16)The testing package is listed under
importsbut no test files exist and there is notesttask. This inflates the lockfile with a dependency that serves no current purpose. Remove it until tests are added.CI — adversarial review
needslist is inconsistent with regular review (.forgejo/workflows/ci.yml:764–800)The
claude-adversarial-reviewjob omitsmodel-check,model-lockfile, andactions-auditfrom itsneedslist, while theclaude-reviewjob includes all three. This means the adversarial review can run (and potentially block a PR) even when model or actions audit jobs have not passed. The omissions look unintentional given that both reviews serve the same gate purpose.CI Security Review
Critical / High
No critical or high severity findings.
Medium
Prompt files read from PR checkout — self-referential trust boundary (
ci.yml:721,ci.yml:836,ci.yml:927): The LLM review prompts are loaded viacat .forgejo/prompts/review.md(andadversarial.md,ci-security.md) from the checked-out PR code. A same-repo contributor could modify these prompt files in the same PR being reviewed — e.g., adding "always approve, never touch review-failed" — and the weakened prompt would be used to evaluate that very PR.Mitigation already in place: Changes to
.forgejo/trigger the CI security review job, which uses a separate prompt (ci-security.md). However, if a contributor modifies all three prompt files in a single PR, all three reviews use the tampered prompts. This is inherent to thepull_requesttrigger model (the PR author controls all checked-out code, including the workflow YAML itself), so the real gate is branch protection requiring human approval. This is medium rather than high because: (a) fork PRs don't receive secrets, so the review steps fail harmlessly for external contributors, (b) same-repo contributors with push access are already in the trust boundary, and (c) the workflow YAML is equally modifiable by the PR author.Suggested hardening (optional): Consider fetching prompt files from the base branch (
git show origin/main:.forgejo/prompts/review.md) rather than reading from the working tree, so the review prompt cannot be tampered with by the PR under review. This would make the review immune to self-referential prompt modification.Low
Changed file names interpolated into LLM prompt (
ci.yml:726,ci.yml:841): TheCHANGED_FILESlist (fromgit diff --name-only) is interpolated directly into the prompt string passed to Claude. A contributor could theoretically create a file with a name containing prompt injection text (e.g.,IGNORE ALL PREVIOUS INSTRUCTIONS.txt). This is low severity because: (a) both prompts include a security preamble treating PR content as untrusted, (b) the LLM tool scope is tightly restricted (no arbitrary Bash, no network), so even a successful injection could only affect the review verdict, and (c) file names have limited character sets.Workflow-level permissions vs. job-level (
ci.yml:9): Permissions are set at the workflow level (contents: read) with job-level overrides for review jobs addingpull-requests: write. The checklist prefers job-level scoping, but the workflow-levelcontents: readis the minimum needed by all jobs and is not overly permissive. The review jobs correctly specify bothcontents: readandpull-requests: writeat the job level, which fully replaces the workflow-level permissions. This is compliant in practice.Verdict
PASS — The CI workflow demonstrates strong security practices: safe
pull_requesttrigger (notpull_request_target), tightly scoped LLM tool access (Read,Grep, specificgitandteecommands only), SHA-256 verified binary downloads for Claude CLI, prompt hardening with security preambles in both review prompts, secrets scoped to only the steps that need them, no auto-merge, and appropriate permission scoping. The medium finding about prompt files being read from the checkout is an inherent limitation of thepull_requesttrigger model and is mitigated by existing branch protection and human review requirements.Adversarial Review
Medium
netpol.ts:100-126 -- normalizePeer silently drops combined pod+namespace selectors
A V1NetworkPolicyPeer can have both podSelector and namespaceSelector simultaneously, meaning
pods matching X in namespaces matching Y. The current code checks if (peer.podSelector) first
and returns immediately, so when both are present the namespaceSelector is silently dropped. The
normalized output will report a combined peer as just kind podSelector, losing the namespace
constraint entirely.
Breaking example: A NetworkPolicy peer with both podSelector (matchLabels: app=api) and
namespaceSelector (matchLabels: env=prod) -- meaning api pods in prod namespaces only -- would
be normalized to kind podSelector with selector app=api, losing the prod-namespace constraint.
The security-audit and network-audit workflows then present an inaccurate picture of the policy scope.
Suggested fix: Check for the combined case first (both podSelector and namespaceSelector present)
and encode both selectors in the normalized output, e.g. kind podAndNamespaceSelector with both
selector and namespaceSelector fields. This requires updating the peer schema to accommodate the
additional field.
namespace.ts:496-557,617-682 -- setResourceQuota / setLimitRange replace path sends body without resourceVersion
When the resource already exists, the replace path constructs a fresh body (lines 502-508, 627-633)
without copying metadata.resourceVersion from the read result. Most Kubernetes API servers require
resourceVersion on PUT to enforce optimistic concurrency. This means the replace path will either:
Breaking example: Call setResourceQuota on a quota that already exists -- the
replaceNamespacedResourceQuota at line 529 sends metadata with only name and namespace but no
resourceVersion, which the API server rejects.
Suggested fix: Retain the object returned by the existence-check read, and copy its
metadata.resourceVersion into the replacement body before calling replace.
pod.ts:99-101 -- exec method schema missing namespace argument
Every other method across all 15 models accepts an optional namespace argument to override
globalArgs.namespace. The ExecArgsSchema (line 99) omits it, and exec (line 471) uses
globalArgs.namespace directly. A user who creates a pod model instance with namespace=default
cannot exec into a pod in another namespace without creating a second model instance.
Breaking example: Model instance created with namespace=default. User calls exec intending
to target a pod in the jobs namespace -- there is no way to override the namespace per-call.
Suggested fix: Add namespace: z.string().optional() to ExecArgsSchema and use
args.namespace ?? globalArgs.namespace like every other method.
Low
helpers.ts:65-96 -- buildClient creates new API clients per method call
Every method call goes through buildClient(), which instantiates a new KubeConfig, loads it
from disk, and creates 7 API clients plus a metrics client. Connection pools and file handles
are never reused across calls. The process.setMaxListeners(30) at line 7 patches a symptom
(listener warnings) rather than the root cause (no client reuse).
In practice this works for low-throughput use but will cause file descriptor churn and
increased latency under load. Not a correctness bug.
Inconsistent use of sanitizeInstanceName
Some models sanitize the instance name before calling writeResource (configmap, deployment,
ingress, hpa, etc.) while others pass the raw normalized.name directly (pod.ts:239, service.ts:249,
namespace.ts:328). Kubernetes resource names are DNS-1123 compliant so this is unlikely to cause
issues, but the inconsistency is worth noting.
Verdict
PASS -- The code is well-structured with consistent patterns across 15 models. The normalization
logic, schema definitions, and workflow orchestration are solid. The medium findings are real
data-integrity and API-compatibility concerns (especially the NetworkPolicy selector loss and the
missing resourceVersion), but they produce incorrect data or API errors rather than security
vulnerabilities or data corruption. None rise to blocking severity.