feat: add @swamp/git extension for CI automation #173
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/swamp-vcs-extension"
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
@swamp/gitmodel extension wrapping the git CLI with structured output for swamp workflowsclone,diff,status,log,commit,push,branch,config— covering every git operation used in this repo's CIswamp dataMotivation
The CI workflows in this repo have ~30 lines of inline bash doing git operations — change detection (
git diff --name-only), status checks (git status --porcelain), and the regenerate-models bot's commit-and-push cycle. This extension makes those operations typed, testable, and reusable in swamp workflows.What's included
Extension (
git/)manifest.yaml— version 2026.08.05.1, 100% quality score (14/14)extensions/models/git.ts— model entrypoint with 8 methods, 8 resources, 2 pre-flight checksextensions/models/_lib/— schemas, operations, runner (with injectable test seam), tracing, types, checksextensions/models/git_test.ts— 63 unit testsCI (3 workflow files)
ci-extensions.yml—git/path trigger, change detection, check/lint/fmt/test jobs, lockfile verification, gate entryci-reviews.yml—git/in source change detection regex for code review triggeringci.yml—gitin outdated dependency scan directory listVerification
deno check/deno lint/deno fmt --check— all cleandeno test— 63 passed, 0 faileddeno install --frozen— lockfile up to dateswamp extension quality— 100% (14/14 points)swamp extension source add+swamp doctor extensions— loads and indexes correctlyswamp model method run— all 8 methods verified with correct structured output:status: porcelain parsing, clean/dirty detectionlog: structured commit entries (sha, author, date, message)config: get and set with correct valuesbranch: list (correct current branch), create (correct created flag)commit: stage + commit with SHA in outputdiff: name-only file list, three-way vs two-way, path filteringclone: shallow clone, branch clone, custom pathpush: normal push, force push with diverged history (verified remote-only file absent after overwrite)Test plan
swamp extension source add git/in a fresh repo loads the extensionswamp model create @swamp/git reposucceedsswamp model method run repo status --jsonreturns structured dataswamp model method run repo diff --input base=HEAD~1 --input nameOnly=true --jsonreturns file listCI Security Review
Critical / High
No critical or high severity findings.
Medium
No medium severity findings.
Low
No low severity findings.
Analysis
This PR adds CI pipeline support for the new
git/extension across all three workflow files. Each change was reviewed against the full security checklist:1. Prompt Injection — No change to LLM prompt construction. The only modification to
ci-reviews.yml(line 34) addsgit/to the grep pattern used for source-change detection, which is a static string in the workflow definition, not user-controlled data.2. Expression Injection — The new
git-checkjob uses${{ matrix.task }}inrun:blocks, but the matrix values are hardcoded to[check, lint, fmt, test](line 444). No attacker-controlled data enters these expressions. SHA values in the change detection step are correctly passed viaenv:(lines 44–46), not interpolated directly.3. Triggers — All workflows use
pull_request(notpull_request_target). No new trigger types are added.4. Supply Chain — The new jobs use
actions/checkout@v6anddenoland/setup-deno@v2, matching all existing jobs. Both are trusted publishers acceptable with tag-only pins per the review policy.5. Permissions —
ci-extensions.ymlhas workflow-levelcontents: readonly. No permissions are added or broadened by this PR.6. Secret Exposure — The new
git-checkandgit-lockfilejobs use no secrets. Theci.ymlchange only addsgitto afindcommand's directory list. No secret handling is modified.7. Auto-merge — No auto-merge logic is added or changed.
Verdict
PASS — Security-neutral change. Adds CI jobs for the
git/extension using the same hardened patterns as all existing extension jobs.Code Review
Blocking Issues
runLogsilently discards custom-format output (git/extensions/models/_lib/operations.ts:278–314)When
args.formatis provided,git logruns with the custom format string andresult.stdoutis populated — but it is never written to the resource. The resource is always written as{ commits: [], count: 0 }:Compare with
runDiffandrunStatus, which both include arawfield in their resource payloads.LogResultSchema(schemas.ts:164–167) also lacks arawfield, so there is nowhere to surface the output.Any caller that sets
args.formatto a custom--formatstring gets back an empty, useless result. Theformatparameter is effectively broken. Fix: addraw?: stringtoLogResultSchemaand populate it inrunLogwhenargs.formatis set (mirroring thediffResult/statusResultpattern).Suggestions
Duplicate import from the same module (
git/extensions/models/_lib/operations.ts:5–15)BranchArgsis imported in a separateimport typestatement from./schemas.tsrather than being merged with the first block. Minor style issue, not a correctness concern.Token may appear in git stderr error messages (
git/extensions/models/_lib/operations.ts:54–58)When
args.tokenis set, the token is embedded in the clone URL. Ifgit clonefails,result.stderris included verbatim in the thrown error:Modern git normally masks credentials in stderr, but this is not guaranteed across all git versions and proxy configurations. Consider sanitizing the URL in error messages (e.g., replace
parsed.passwordwith***for the error string) to prevent accidental token exposure in logs.runConfigset logs the config value (git/extensions/models/_lib/operations.ts:603)ctx.logger.info(set ${args.key} = ${args.value})logs the full config value. For standard use (user.name,user.email) this is harmless, buthttp.extraHeaderorurl.<base>.insteadOfcan carry tokens. Consider logging only the key.Adversarial Review
Critical
Token leakage in clone error messages and OpenTelemetry spans
git/extensions/models/_lib/operations.ts:42-47,56-58,80-83When
args.tokenis provided, the token is embedded in the clone URL viaparsed.password = args.token. If the clone then fails (auth error, networktimeout, invalid repo, etc.), the error path includes
result.stderrin thethrown error message, which typically contains the full URL with the embedded
credentials.
Git stderr for a failed clone regularly includes the full URL, for example:
fatal: unable to access 'https://x-access-token:ghp_abc123@github.com/org/repo/': ...This error message then propagates to the OpenTelemetry span at line 80-83
via
span.setStatus, which is exported to observability backends.The token is explicitly marked
sensitive: truein the schema(
schemas.ts:33), but a failed clone leaks it through:Errorobject (visible to any upstream catch handler or logger)Breaking example: Clone with a token against a repo that returns 401.
The stderr contains the full URL with the token embedded, and that string
lands in traces and error logs.
Suggested fix: Scrub the URL from stderr before including it in the
error message. Redact anything matching
https://[^@]*@back tohttps://***@, or do not include stderr at all for authenticated clones.Also do not pass the raw error message into
span.setStatus.High
Custom log format silently discards all output
git/extensions/models/_lib/operations.ts:270-300When
args.formatis provided (a supported schema field), the raw git outputis captured in the
rawvariable (line 270) but the resource is written withonly
commits(empty array) andcount(0). Therawoutput is neverincluded in the resource. Compare with
runDiff(line 142) which correctlyincludes
rawin its resource. TheLogResultSchemaalso has norawfield,so this is a schema + implementation gap.
Breaking example: Calling log with a custom format string returns a
resource with commits=[] and count=0. The actual git output is computed,
stored in a local variable, and thrown away. The caller gets an empty result
with no way to access the data.
Suggested fix: Add a
rawfield toLogResultSchemaand includerawinthe resource output, matching the pattern used by
runDiff.Medium
Log commit parsing corrupted by delimiter strings in commit subjects
git/extensions/models/_lib/operations.ts:234-290The structured log parser splits on literal delimiter strings
---GIT_LOG_ENTRY---and---FIELD---. If any commit subject linecontains either of these strings, the split produces incorrect results.
For
---FIELD---in a subject,parts[3]is truncated and the remainderbecomes a spurious
parts[4]that is silently dropped. For---GIT_LOG_ENTRY---in a subject, a single commit is split into twoentries, both with corrupted fields.
Breaking example: A commit with message containing
---FIELD---in thesubject produces a truncated message field with the rest silently lost.
Suggested fix: Use NUL bytes as the field delimiter in the git format
string and split on NUL. NUL bytes cannot appear in commit messages. Or limit
the split with a max-splits argument and join remaining parts to prevent
data loss for the last field.
Token silently ignored for non-HTTPS URLs
git/extensions/models/_lib/operations.ts:42If a caller passes
tokenwith a URL starting withhttp://instead ofhttps://, the token is silently discarded because of thestartsWithguard. The clone proceeds without auth and fails with a confusing error.
While refusing to send tokens over plaintext is correct, doing so silently
wastes debugging time.
Suggested fix: Throw an explicit error if
tokenis provided with anon-HTTPS URL.
Low
Clone path derivation fragile for unusual URLs
git/extensions/models/_lib/operations.ts:61-62The clone path derivation via
args.url.split("/").pop()does not handletrailing slashes or query parameters correctly. A URL with query params
would produce an incorrect cloned path in the resource metadata.
Verdict
FAIL -- the token leakage in clone error messages (Critical #1) is a
credential exposure bug in a production code path. The custom-format log
discarding output (High #2) is a functional correctness bug. Both should be
fixed before merge.
CI Security Review
Critical / High
No critical or high severity findings.
Medium
No medium severity findings.
Low
No low severity findings.
Analysis
This PR makes three mechanical changes to add
git/extension support to the existing CI pipeline:ci-extensions.yml: Addsgit/to path triggers, change detection outputs, and path-check logic. Addsgit-checkandgit-lockfilejobs following the identical pattern used by all other extensions (agent-runner, ssh, kubernetes, etc.). Adds these jobs to the gate'sneedslist. No new secrets, no new permissions, no new actions introduced.ci-reviews.yml: Addsgit/to the grep pattern that detects source changes warranting code review (line 34). No changes to LLM prompts, tool scoping, or secret handling.ci.yml: Addsgitto thefindcommand's directory list for the outdated dependency check (line 29). No other changes.Security checklist results:
Read,Grep,Bash(git diff:*),Bash(git log:*), and specifictee/touchcommands), andWrite/Edittools explicitly disallowed.git-checkjob uses${{ matrix.task }}inrun:blocks, but these are hardcoded matrix values ([check, lint, fmt, test]), not attacker-controlled input. TheBASE_SHA/HEAD_SHAvalues are passed safely viaenv:blocks and quoted in shell. This matches the existing safe pattern used by all other extension jobs.pull_request(notpull_request_target), which runs the workflow definition from the base branch.actions/checkout@v6anddenoland/setup-deno@v2are from trusted publishers, consistent with project policy.contents: readonly inci-extensions.ymlandci.yml. No permissions changes in any file. The new jobs inheritcontents: readonly.git-checkandgit-lockfilejobs use no secrets. Noenv:blocks referencesecrets.*.Verdict
PASS — Security-neutral changes that mechanically extend existing CI patterns to cover a new
git/extension directory. No new attack surface introduced.Code Review
Blocking Issues
None.
Suggestions
runStatusis incorrect whenporcelain: false(operations.ts:221–223):cleanis determined by counting non-empty lines in the raw output. For non-porcelaingit status, the output always has multiple header lines even on a clean repo, socleanwould always befalse. Sinceporcelaindefaults totruethis doesn't affect the documented usage path, but a caller who explicitly passesporcelain: false(expecting thecleanflag to still be meaningful) would get wrong results. The schema default oftruenudges callers away from this case, but a guard or note in the schema description would prevent future confusion.Missing exit-code check after
rev-parse HEADinrunCommit(operations.ts:377–378): After a successfulgit commit,rev-parse HEADis called without checkingshaResult.exitCode. If it fails for any reason,shasilently becomes an empty string and thecommitResultresource is written withsha: "". Low practical risk (HEAD always resolves after a commit), but a quick guard here (if (shaResult.exitCode !== 0) throw ...) would keep the error surfacing consistent with the rest of the file.Unused
@systeminit/swamp-testingdependency (deno.json): The package is declared underimportsbut never imported ingit_test.ts(the vault/datastore conformance helpers don't apply to a git extension). Remove it to keep the dependency surface minimal and the lock file honest.Missing test for
config geterror path (git_test.ts): Every other operation has a "throws on failure" test, butrunConfigin get-mode has no coverage for thegit config get failedbranch. A one-liner test withsetCommandExecutor(() => fail("key not found"))would close the gap.--allow-writepermission in tests appears unnecessary (deno.jsontask,ci-extensions.yml): The test suite uses an in-memory harness —writeResourcepushes to an array, and no test writes to the filesystem. If--allow-writewas copied from another extension's template, it can be dropped.Adversarial Review
Critical / High
(none)
Medium
Status porcelain parsing destroys positional status codes — git/extensions/models/_lib/operations.ts:211
Git porcelain v1 format encodes index vs worktree status in a two-character XY field where position matters. " M" means modified in worktree only (unstaged), "M " means modified in index only (staged), "MM" means modified in both. The code does:
.trim() collapses " M" and "M " both into "M", losing the staged-vs-unstaged distinction. Any consumer that needs to distinguish staged from unstaged changes gets incorrect data.
Breaking input: git status --porcelain returns " M src/main.ts" for an unstaged modification and "M src/main.ts" for a staged modification. Both produce status:"M" and path:"src/main.ts" — indistinguishable.
Suggested fix: Remove .trim() and preserve the raw two-character status code:
Git flag injection via user-controlled string arguments — git/extensions/models/_lib/operations.ts (multiple locations)
Several operations pass user-controlled strings directly as positional git arguments without ensuring they do not start with a dash. If an upstream caller allows untrusted input (e.g., PR branch names) to flow into method arguments, dash-prefixed strings are interpreted as git flags instead of refs/branch names.
Affected paths:
The risk is conditional — requires the caller to pass unsanitized user input into method args. In typical CI usage with hardcoded refs/SHAs this is not exploitable. But the extension provides no input validation or documentation warning.
Suggested fix: Add a Zod refinement to ref/branch/key schemas that rejects strings starting with a dash. Or prepend --end-of-options before positional ref arguments (requires Git 2.39+).
Low
Diff count is zero in non-nameOnly modes — git/extensions/models/_lib/operations.ts:148,160
When nameOnly is false (including stat mode), files is always an empty array and count is always 0, even though the diff contains changed files. A consumer checking count === 0 to determine "no changes" would get a false positive when using stat mode.
Example: diff with stat: true produces files:[], count:0, raw:" 3 files changed, 10 insertions(+)" — count says 0 but raw shows 3 files changed.
Verdict
PASS — The code is well-structured with thorough tests, proper credential scrubbing, correct tracing/span lifecycle, and clean CI integration. The status parsing issue (Medium 1) silently loses data but will not crash, and the flag injection issue (Medium 2) requires a specific attack path that typical CI usage will not hit. Neither rises to blocking severity. Both are worth fixing before this extension is used in pipelines that route untrusted input into method arguments.
CI Security Review
Summary
This PR adds CI support for a new
git/extension across three workflow files. The changes are mechanical — they replicate the exact patterns used for every other extension (agent-runner,ssh,kubernetes, etc.) and addgit/to existing path filters and directory scans.Detailed Analysis
1. Prompt Injection — No new LLM invocations introduced. The only change to
ci-reviews.ymlis appending|git/to the source-change detection regex (line 34). The review prompt construction, tool scoping, and prompt files are unchanged and out of scope.2. Expression Injection — The new
git-checkjob interpolates${{ matrix.task }}in arun:block (ci-extensions.yml:455). The matrix values are hardcoded literals[check, lint, fmt, test](line 444) — not attacker-controlled. This is identical to every other*-checkjob in the file. No event fields (issue title, PR body, commit messages, etc.) are interpolated inrun:blocks in any changed code.3. Dangerous Triggers — All workflows use
pull_request(notpull_request_target). Noissue_comment,workflow_dispatch, or other externally-triggerable events. Safe.4. Supply Chain — The new jobs use
actions/checkout@v6anddenoland/setup-deno@v2, both from trusted publishers (GitHub-owned and Deno). No new third-party actions, nocurl | bash, no remote script execution.5. Permissions —
ci-extensions.ymlandci.ymluse workflow-levelpermissions: contents: read, which is appropriate since all jobs in these workflows only need read access. No permissions escalation introduced. Theci-reviews.ymlchange does not modify any permissions blocks.6. Secret Exposure — The new
git-checkandgit-lockfilejobs reference no secrets. They run onlydeno check,deno lint,deno fmt --check,deno test, anddeno install --frozen. Theci.ymlchange addsgitto afindcommand directory list with no secret involvement.7. Auto-merge & Trust Boundaries — No auto-merge behavior introduced. The gate job addition (
git-check,git-lockfileadded toneeds:) is the standard failure-checking pattern.Verdict
PASS — All changes are security-neutral, mechanically extending existing safe patterns to cover a new
git/extension directory. No new attack surface introduced.Code Review
Blocking Issues
git addpaths missing--separator inrunCommit(operations.ts:353)Every other path-accepting operation in this file correctly inserts
--before paths to prevent flag injection (runStatus,runDiff,runLogall do this).runCommitdoes not. If any element ofargs.pathsstarts with-, git interprets it as an option rather than a path — e.g.,--patchwould trigger interactive patch mode (which would fail/hang with piped stdio), and--dry-runwould silently stage nothing. The fix is one token:The
CommitArgsSchema.pathsusesz.array(z.string()).optional()with no dash-prefix guard (unlikesafeRef/safeRefOptional), so this is the only line of defense.Suggestions
Token visible in process argument list (
operations.ts:57) — The token-embedded URL (https://x-access-token:<token>@host/...) is passed as a plain argv element toDeno.Command. It's correctly scrubbed from error messages and never written to logs or resources, but the raw URL does appear in/proc/<pid>/cmdlinefor the duration of the git subprocess. In shared-node CI environments this could be a concern. UsingGIT_ASKPASSor a credential helper would avoid this, though the current approach is standard practice for disposable CI runners.diffFilteraccepts arbitrary strings without validation (schemas.ts:61) — Since it's bundled into a single argv element (--diff-filter=<value>) there is no shell injection risk, but an invalid filter string (e.g.,!) would cause git to error rather than failing at parse time. Validating against the known filter characters ([ACDMRTUXBacmdmrtuxb*]) would give a clearer error before a subprocess is spawned.manifest.yamlrepositoryfield points to GitHub —repository: "https://github.com/swamp-club/swamp-extensions"but the upstream is atgit.swamp-club.com. This won't affect runtime behavior but may be confusing for consumers.Adversarial Review
Critical / High
CRITICAL -- Clone URL flag injection (git/extensions/models/_lib/schemas.ts:39, git/extensions/models/_lib/operations.ts:57)
CloneArgsSchema.url is validated as z.string().min(1) with no dash-prefix check, unlike every ref-like field that uses safeRef. The URL is pushed directly into the git argv array. A caller passing url "--upload-pack=/tmp/evil" produces git clone --upload-pack=/tmp/evil, which executes /tmp/evil as the upload-pack program -- arbitrary command execution.
Breaking input: url set to "--upload-pack=/bin/sh"
Fix: Add a safeRef-style refinement to url, or push -- before the url in the argv. The -- approach is more robust since it protects path too.
HIGH -- Commit paths missing -- separator (git/extensions/models/_lib/operations.ts:353)
runCommit stages files with git add ...args.paths but omits the -- separator that runDiff (line 133), runStatus (line 196), and runLog (line 269) all include. A path element starting with - (e.g. --chmod=+x or --intent-to-add) would be interpreted as a git add flag rather than a filename.
Breaking input: message "test", paths ["--chmod=+x", "script.sh"]
Fix: Insert "--" before the spread: execGit(["add", "--", ...args.paths]).
HIGH -- Clone path argument allows flag injection (git/extensions/models/_lib/schemas.ts:42, git/extensions/models/_lib/operations.ts:59-61)
CloneArgsSchema.path is z.string().optional() with no dash-prefix check and no -- separator in the argv. Since path is appended after url, a value like --template=/tmp/evil-templates would be interpreted as a git clone flag, not a destination directory. The --template flag causes git to copy files from the template directory into .git/, which can include hook scripts that execute automatically.
Breaking input: url "https://example.com/repo", path "--template=/tmp/evil"
Fix: Use argv.push("--", url) to terminate option parsing before positional arguments, or add safeRef to the path field.
Medium
MEDIUM -- AbortSignal declared but never wired (git/extensions/models/_lib/types.ts:14, git/extensions/models/_lib/runner.ts:11-16)
GitContext exposes signal: AbortSignal but no operation passes it to Deno.Command. A long-running git clone on a slow or unresponsive remote cannot be cancelled. Deno.Command supports an AbortSignal via its constructor options.
Not a correctness bug, but a contract violation -- callers that set signal expect cancellation to work.
Low
LOW -- Porcelain rename entries produce composite path strings (git/extensions/models/_lib/operations.ts:211-214)
line.substring(3) captures the full text after the XY+space in porcelain v1 output. For renames/copies, git outputs R old -> new, so the path field would contain old -> new as a single string. Consumers expecting a clean file path may be surprised. Not a bug since the raw field has the full output, but the entries array is less useful for renames than it could be.
LOW -- Module-level mutable executor in runner.ts (git/extensions/models/_lib/runner.ts:27)
The executor variable is mutable module-level state used for test injection. This is standard practice and tests use try/finally to restore, but if this module were ever imported by concurrent production code paths, the test helpers setCommandExecutor/resetCommandExecutor could interfere. In practice this is fine since test helpers would not be called in production.
Verdict
FAIL -- Three flag injection vectors (clone URL, clone path, commit paths) allow untrusted input to inject git flags. The clone URL case is particularly severe as --upload-pack enables arbitrary command execution. The commit paths issue is a clear inconsistency with the -- separator pattern used by all other path-accepting operations.
CI Security Review
Summary
This PR adds CI coverage for the new
git/extension directory across three workflow files. All changes are mechanical additions that follow established patterns exactly:git/**to path triggers, agitoutput to the change-detection job, and two new jobs (git-check,git-lockfile) that are structurally identical to the existing extension jobs. Adds both to the gate.git/to the grep pattern that detects source changes warranting code review.gitto thefindcommand's directory list for the outdated-dependency check.Checklist Results
${{ matrix.task }}with hardcoded matrix values[check, lint, fmt, test]— not attacker-controlledpull_request(notpull_request_target); no new triggers addedactions/checkout@v6anddenoland/setup-deno@v2used — both trusted publishers per policycontents: readonly; no escalationVerdict
PASS — Security-neutral changes. The new
git/extension CI jobs are exact copies of the established pattern with no new attack surface, permissions, secrets, or triggers introduced.Code Review
Blocking Issues
Flag injection in
git configvia unvalidatedvalueparameter (git/extensions/models/_lib/schemas.ts:1441,git/extensions/models/_lib/operations.ts:1196–1199)ConfigArgsSchema.valueaccepts any string (z.string().optional()) with no check against leading dashes. Thekeyfield correctly validates!v.startsWith("-")with an explicit comment about flag interpretation, butvaluehas no equivalent guard.When
git configis called as:git's option parser is still active at that position — it does not stop at positional arguments by default. A caller providing
value: "--unset"would produce:which silently deletes the key instead of setting it. Similarly
"--unset-all"deletes all matching entries, and"--global"could override the intended scope. There is no--separator support ingit configto prevent this.The existing injection-prevention tests in
git_test.ts(lines 951–978) coverdiff.base,push.branch,config.key, andbranch.name, but there is no test forconfig.value, confirming the gap.Fix: Apply the same
!v.startsWith("-")refinement tovalueinConfigArgsSchema:And add a corresponding schema-rejection test.
Suggestions
runStatuswithporcelain=falseproduces a misleadingcleanflag (git/extensions/models/_lib/operations.ts:808)When
porcelain=false,git statusalways prints human-readable prose (e.g., "nothing to commit, working tree clean"), soclean: lines.length === 0is alwaysfalseeven for a clean tree. Sinceporcelaindefaults totrueand non-porcelain output isn't used in any documented path, this edge case is unlikely to cause problems in practice. Options: document thatcleanis only meaningful withporcelain=true, or rejectporcelain=falseexplicitly since structured parsing isn't supported in that mode.clonelogs original URL including user-supplied URLs (git/extensions/models/_lib/operations.ts:664)ctx.logger.info(cloned ${args.url} to ${clonedPath})logsargs.url(the caller-supplied URL). The token-embedding logic correctly uses a localurlvariable, so the token never appears here. However, if a caller embeds credentials directly in the URL string (bypassing thetokenparam), they would appear in the log. This is a mis-use, but adding ascrubCredentials()call to thelogger.infoline (consistent with how error messages are scrubbed) would provide defense in depth at negligible cost.Adversarial Review
Critical / High
HIGH —
git logdefault format uses literal NUL bytes in command argument, which Unix cannot pass to a subprocess (git/extensions/models/_lib/operations.ts:261)The format string is constructed with literal
\x00bytes embedded in the argument string:On Unix, command-line arguments are null-terminated C strings — a NUL byte inside an argument is impossible to pass through
execvp. Deno delegates to Rust'sstd::process::Command, which callsCString::new(arg)before exec. That function returnsErr(NulError)when the arg contains an interior NUL, soDeno.Command.output()will reject the argument at spawn time.Breaking scenario: any call to
model.methods.log.execute({}, ctx)(i.e. without a customformat) will throw a low-level spawn error instead of returning commit history. This is the primary usage path. All tests pass because they mock the command executor and never invoke a real process.Suggested fix: use git's own hex escape,
%x00, which git interprets as a NUL byte in the output without requiring a literal NUL in the argument:The NUL-based parsing of the output remains correct — only the argument construction needs to change.
Medium
MEDIUM —
GlobalArgsSchema.remotelacks thesafeRefrefinement applied to per-method ref fields (git/extensions/models/_lib/schemas.ts:10-11)Per-method schemas correctly use
safeRef/safeRefOptionalto reject values starting with-, preventing flag injection. But the globalremotefield is plainz.string().default("origin"):In
runPush(operations.ts:427), the global remote is used as a fallback:If
globalArgs.remoteis set to--mirror, the resulting command isgit push --mirror <branch>, which performs a mirror push — deleting remote branches that don't exist locally.Suggested fix: add the same refinement to
GlobalArgsSchema.remote:Low
LOW —
scrubCredentialsonly covershttps://…@patterns (git/extensions/models/_lib/operations.ts:22-24)The regex
https:\/\/[^@]*@correctly handles the URL-embedded credentials thatrunCloneproduces. But if a future code path ever logs a token outside of a URL context (e.g., in a header or diagnostic), the scrubber won't catch it. This is not exploitable today —runCloneis the only operation that handles tokens, and its error paths correctly apply the scrubber. Noting for awareness only.Verdict
FAIL — the
git logNUL-byte issue (#1 High) makes the default-format log operation non-functional against a real git binary. The tests pass only because the command executor is mocked. This should be fixed before merge.CI Security Review
Summary
This PR extends CI coverage to the new
git/extension across all three workflow files. The changes are purely additive and follow the exact same patterns as the existing extension jobs.Changes reviewed:
git/**path trigger, change detection output,git-checkandgit-lockfilejobs, and includes them in the gate.git/to the source-change grep pattern so code reviews trigger for git extension changes.gitto thefinddirectory list for the outdated dependency check.Medium
*-checkjobs) —${{ matrix.task }}is interpolated directly in arun:block (e.g.,if [ "${{ matrix.task }}" = "fmt" ]). While the matrix values are hardcoded to[check, lint, fmt, test]and not attacker-controlled, the safer pattern is to pass the value via an environment variable (env: TASK: ${{ matrix.task }}). This is a pre-existing pattern across all extension jobs, not introduced by this PR, but the newgit-checkjob replicates it. Not exploitable given the hardcoded matrix, but noted for defense-in-depth.Low
actions/checkout@v6anddenoland/setup-deno@v2use tag-only pins rather than full SHA pins. Per the review policy,actions/*anddenoland/*are trusted publishers where tag pins are acceptable. No action required.Verdict
PASS — All three files contain minimal, additive changes that extend existing CI patterns to cover the new
git/extension. No new secrets are introduced, no new triggers, no new LLM integrations, and no user-controlled data flows into shell commands or expressions. The changes are security-neutral.Code Review
Blocking Issues
Credential leak in cloneResult resource storage
File: git/extensions/models/_lib/operations.ts, lines 83 and 87
The logger at line 78 correctly applies scrubCredentials before writing to
the log, but the subsequent writeResource call passes args.url unscrubbed
into both the resource body (url field) and the resource tags (tags.url).
If a caller passes credentials directly in the URL string (for example
https://user:pass@github.com/org/repo) rather than using the separate token
field, those credentials are stored verbatim in the persisted resource data
and tags. The token field is the intended mechanism and the normal path is
clean, but CloneArgsSchema.url accepts any non-empty string with no
constraint against embedded credentials. The scrubCredentials function
exists precisely for this risk; it must be applied consistently wherever
args.url is persisted, not only in the logger.
Fix: apply scrubCredentials(args.url) in both the resource body url field
and the tags.url field.
There is also no test covering this path (credentials embedded in the URL
string rather than via token), so a test should be added alongside the fix.
Suggestions
diffFilter accepts arbitrary strings with no character validation
(schemas.ts line 64). Since the value is embedded as a single argv entry
via Deno.Command (no shell), there is no injection risk at runtime. A regex
constraint restricting to valid git diff-filter characters (A, C, D, M, R,
U, X, asterisk, digits) would surface malformed inputs early with a clear
schema error rather than a confusing git failure.
log.format is also unvalidated (schemas.ts line 89). Same reasoning: no
injection via array-based Deno.Command, but explicit validation would
produce better error messages for callers.
CI workflow prompt variable quoting (ci-reviews.yml lines 116, 223, 330
and matching jobs in ci-extensions.yml): the PROMPT variable is assembled
by concatenating the prompt file contents and the CHANGED_FILES list, then
passed as: claude -p "$PROMPT". If any changed filename contains a literal
double-quote character, the shell boundary breaks and the argument is
mis-parsed. Writing the prompt to a temp file and referencing it via a
file-path argument would eliminate this class of risk entirely.
config --global can set security-sensitive keys such as core.sshCommand
and credential.helper, which execute arbitrary commands on the next git
network operation. The safeRef guard only prevents leading dashes; it does
not block dangerous key names. A documentation note in the schema, or a
runtime warning when scope=global is combined with known sensitive key
prefixes, would reduce the blast radius of accidental misuse.
--allow-run is unnecessary for tests (git/deno.json and ci-extensions.yml
line 460): the test suite replaces the executor via setCommandExecutor
before any operation executes, so denoExecutor (the function that actually
spawns subprocesses) is never invoked. Removing --allow-run from the test
invocation tightens the sandbox and would surface any test that accidentally
omits the mock.
Branch switch uses git checkout NAME without -- (operations.ts line 562).
For the create path (-b) the argument position is unambiguous. For plain
switch, git prefers branches over file paths so this is safe in practice,
but git switch NAME (git >= 2.23) is semantically cleaner and removes the
latent ambiguity.
Adversarial Review
Critical / High
None found.
Medium
Branch listing returns bogus entry on detached HEAD — operations.ts:504-514
When HEAD is detached, git branch --list outputs a line like
* (HEAD detached at abc1234). The parser at line 509 treats anyline starting with
*as the current branch, socurrentbecomes"(HEAD detached at abc1234)"and that string is also pushed intothe
branchesarray as if it were a real branch name.Breaking example: A CI workflow that clones at a specific SHA
(detached HEAD), then calls
branch listand iteratesbranches— it would see a phantom branch name containing parentheses and
spaces, which would fail if passed back to
branch create/switchor used in downstream logic.
Suggested fix: Detect lines matching
* (HEAD detached at ...)or
* (no branch)— setcurrenttoundefinedfor those andexclude them from the
branchesarray.Status porcelain parsing breaks on filenames containing newlines
— operations.ts:208-214
The parser splits
result.stdoutby newline. Porcelain v1 format(without
-z) is newline-delimited, so a tracked file whose namecontains a literal newline (valid on Linux) would split into two
lines — producing one entry with a truncated path and wrong XY
status code and one garbage entry.
Suggested fix: Use
--porcelain -z(NUL-delimited output) andsplit on NUL instead. This also handles filenames with other special
characters that git may C-quote in porcelain v1 mode.
AbortSignal from context is never threaded to subprocesses —
runner.ts:8-24, types.ts:14
GitContext.signalexists butdenoExecutornever passes it toDeno.Command. A caller settingsignalto request cancellation(e.g., on workflow timeout) has no effect — a slow
git cloneorgit pushwill run to completion regardless.Suggested fix: Thread
signalthroughexecGitintoDeno.Commandvia thesignaloption, and handleAbortErrorinthe operations.
Low
scrubCredentials only strips https credential URLs —
operations.ts:22-24
The regex only matches
https://schemes. While the code enforceshttps://for token injection, git itself could emithttp://URLswith embedded credentials in error messages (e.g., after a redirect
or from a misconfigured remote).
Suggested fix: Broaden the regex to match both
http://andhttps://schemes.Log parsing silently corrupts if a commit subject contains a NUL
byte — operations.ts:248,291
The log format uses NUL as a field separator. If a commit subject
contains a literal NUL, the split-by-NUL logic would shift all
subsequent field boundaries, producing commits with wrong
SHA/author/date/message associations. Extremely unlikely in practice
but theoretically possible.
Status porcelain parsing does not decode C-quoted filenames —
operations.ts:211-214
When
core.quotePathis true (the default), filenames withnon-ASCII characters are C-quoted in porcelain v1 output. The parser
returns the quoted form including surrounding double-quotes, which
would confuse downstream path comparisons. Using
-z(as suggestedin Medium finding 2) would also fix this.
Verdict
PASS — The code demonstrates solid security practices: flag
injection is prevented via Zod safeRef refinements on all ref-like
inputs, clone uses
--before positional arguments, credentials arescrubbed from error messages and logs, and config values are excluded
from log output. The test suite is thorough with 50+ test cases covering
happy paths, error paths, and injection prevention. The medium findings
are edge cases in output parsing (detached HEAD, newline filenames) and
a missing cancellation path — none produce security vulnerabilities or
data loss in common CI usage.
CI Security Review
Summary
This PR adds CI coverage for the new
git/extension directory across three workflow files. All changes are mechanical additions that replicate existing, already-reviewed patterns:ci-extensions.yml: Addsgit/**to path triggers, agitoutput to the change-detection job,check_path git "git/"to the filter script, newgit-checkandgit-lockfilejobs (identical structure to all other extension jobs), and includes them in the gate.ci-reviews.yml: Addsgit/to the grep regex for detecting source changes that warrant Claude code review.ci.yml: Addsgitto thefindcommand's directory list for outdated dependency scanning.Checklist Results
1. Prompt Injection: No new LLM invocations introduced. The only change to the reviews workflow (
ci-reviews.yml:34) expands the grep pattern for change detection — the LLM prompts, tool scoping, and security preambles are unchanged.2. Expression Injection: The new
git-checkjob uses${{ matrix.task }}in arun:block (ci-extensions.yml:454). This is safe becausematrix.taskvalues are hardcoded in the workflow YAML ([check, lint, fmt, test]) and cannot be influenced by external input. This is the same pattern used by all other extension jobs in this file.3. Dangerous Triggers: No new triggers introduced. All three workflows continue to use
pull_request(notpull_request_target), which runs in the fork context without access to base-repo secrets. Safe.4. Supply Chain: No new third-party actions introduced. The new jobs use the same
actions/checkout@v6anddenoland/setup-deno@v2already present throughout the file. These are from trusted publishers (GitHub-owned and Deno-owned respectively), acceptable with tag-only pins per the review policy.5. Permissions: No permission changes. Workflow-level
contents: readremains, which is the minimum needed. The new jobs inherit this — no escalation.6. Secret Exposure: No new secrets introduced or referenced. The new jobs do not access any secrets.
7. Auto-merge & Trust Boundaries: No changes to merge automation or trust boundaries. The gate job correctly includes the new
git-checkandgit-lockfilejobs.Verdict
PASS — All three changes are safe, mechanical extensions of existing CI patterns to cover the new
git/extension directory. No new attack surface introduced.Code Review
This is a well-structured new extension. The test suite is comprehensive, all operations use array-based subprocess spawning (no shell injection risk), credential scrubbing is applied consistently, and flag-injection guards cover every user-supplied ref/remote/branch/key argument. The CI wiring follows the existing patterns correctly.
Blocking Issues
runStatus:cleanflag is wrong whenporcelain=false(git/extensions/models/_lib/operations.ts, lines 795-814)cleanis computed fromlines.lengthbefore branching on porcelain mode. Whengit statusis run without--porcelain, it always emits human-readable header lines (e.g. "On branch main" / "nothing to commit, working tree clean") that makelines.lengthnon-zero even for a clean repo:Since
StatusArgsSchemaexposesporcelainas a user-configurable boolean (defaulttrue), a caller who explicitly passesporcelain: falsealways seesclean: falseregardless of actual working tree state. Fix: either (a) removeporcelainfrom the public schema and always pass--porcelaininternally, or (b) gatecleanonargs.porcelainand omit/undefined it when porcelain mode is inactive.Suggestions
CloneArgsSchema.pathlacks a dash-prefix refinement (_lib/schemas.ts, ~line 1351)Every other positional user input (
base,head,branch,remote,name,startPoint,key,value) has asafeRef/safeRefOptionaldash-prefix refinement.pathdoes not. In practice it is harmless becausepathis placed after--in argv, so git treats it as positional, not as a flag. Adding the same refinement makes the security intent consistently explicit across all user-supplied args.runBranchusesgit checkoutwithout--for branch switching (operations.ts, ~lines 1121-1154)Both
git checkout -b <name>(create) andgit checkout <name>(switch) omit the--separator. Without--, branch names that match tracked file paths produce ambiguous behavior. The moderngit switch/git switch -ccommands eliminate this ambiguity and make the intent clearer. No security impact given thesafeRefguard, but a robustness improvement.checks.tsuses anas stringcast (_lib/checks.ts, ~line 564)const repoPath = (ctx.globalArgs.repoPath as string) || "."uses a bare cast rather than schema parsing. UsingGlobalArgsSchema.parse(ctx.globalArgs).repoPathis consistent withoperations.tsand type-safe.manifest.yamlrepositoryfield points to GitHub (git/manifest.yaml, line 40)The field contains
https://github.com/swamp-club/swamp-extensions. CLAUDE.md states the canonical upstream is the Forgejo instance atgit.swamp-club.com. If this field is a source-of-truth pointer it should reference the Forgejo URL used infgjPR operations.runConfigallows semantically sensitive config keys (operations.ts, ~lines 1196-1221)ConfigArgsSchema.keyis validated only for dash-prefix injection. It permits setting keys such ascore.hookspath,include.path, andcore.gitProxy, which can redirect git hooks or config inclusion to attacker-controlled paths on subsequent operations in the same repo. In a fully trusted CI automation context this is probably acceptable by design, but worth a deliberate decision and a brief README note.Adversarial Review
Medium
Status porcelain parsing produces incorrect results for renamed/copied files
git/extensions/models/_lib/operations.ts:211-215The porcelain parser assumes every line has the format XY-space-path. When git status --porcelain reports renamed or copied files, the output format is XY orig_path -> new_path. The parser puts the whole thing into the path field as a single string, losing the semantic distinction between source and destination paths.
Breaking example: Rename a file (git mv old.ts new.ts), then call status with porcelain true. The resulting entry would have status "R " and path "old.ts -> new.ts" -- a consumer checking entry.path for "new.ts" would fail, and any logic that uses the path to read the file would try to open a nonexistent path.
Suggested fix: Either document that renames produce the combined path string, or detect the R/C status codes and split on " -> " (or better, use --porcelain -z which uses NUL-separated paths and handles filenames with special characters safely).
scrubCredentials only matches https:// URLs -- credentials in http:// URLs leak into error messages
git/extensions/models/_lib/operations.ts:22-24The regex only matches https:// prefixes. If a user passes http://user:password@host/repo as the clone URL (without the token parameter), the clone will proceed (the https check only gates token-based auth). If it fails, scrubCredentials(result.stderr) will not match the http:// prefix and the credentials will appear verbatim in the thrown Error message, the span status, and the log.
Breaking example: runClone with url http://deploy:s3cret@internal.corp/repo.git fails -- error message contains the secret.
Suggested fix: Change the regex to match both http and https protocols.
Low
AbortSignal from GitContext is never forwarded to the subprocess
git/extensions/models/_lib/runner.ts:12-18GitContext exposes a signal: AbortSignal field, but denoExecutor never passes it to Deno.Command. A long-running git clone or git log on a large repo cannot be cancelled by the caller. Deno.Command supports a signal option that would make this work.
diffFilter accepts arbitrary strings without validation
git/extensions/models/_lib/schemas.ts:63Unlike base, head, remote, etc., the diffFilter field has no refinement. It is concatenated with = so flag injection is not possible, but arbitrary strings would produce confusing git errors rather than a clean schema validation failure. Consider restricting to the valid filter characters (ACDMRTUXB*).
Verdict
PASS -- The code is well-structured with strong security posture: consistent safeRef refinements to block flag injection, -- separators on positional arguments, credential scrubbing across error messages/logs/spans/resources, and good test coverage (~50 tests including injection prevention). The medium findings affect edge cases (renamed files in status, http credentials) and are not blocking.
CI Security Review
Summary
This PR adds CI pipeline support for a new
git/extension directory across three workflow files. The changes follow the identical pattern already established for all other extensions (agent-runner, ssh, kubernetes, etc.):git/**path trigger, change detection,git-check/git-lockfilejobs, and gate integration.git/to the source-change grep filter so the existing LLM review jobs cover it.gitto thefinddirectory list for the outdated-dependency check.Checklist Results
git/as a source path — file paths only, no user-controlled content interpolated${{ matrix.task }}used inrun:blocks, but values are hardcoded in the matrix ([check, lint, fmt, test]), not attacker-controlledpull_request(notpull_request_target), safeactions/checkout@v6anddenoland/setup-deno@v2— trusted publishers, acceptable with tag pins per policycontents: readon ci-extensions.yml and ci.yml (appropriate — all jobs are read-only). ci-reviews.yml unchanged jobs have job-levelpull-requests: writescopingCritical / High
(none)
Medium
(none)
Low
(none)
Verdict
PASS — Security-neutral change. The new
git/extension jobs are structurally identical to all existing extension CI jobs, introducing no new attack surface, triggers, permissions, secrets, or LLM interactions.Code Review
Blocking Issues
None.
Suggestions
CloneArgsSchema.pathmissing leading-dash guard (git/extensions/models/_lib/schemas.ts:44)All other positional string args (
remote,branch,base,head,startPoint, configkey/value) validate against a leading dash.pathis missing this refine. In practice it is safe becausepathis placed after the--separator and the URL in the argv array, so git won't treat it as a flag — but the inconsistency is a readability / future-safety concern. Adding the same!v.startsWith("-")refine would make the invariant uniform.Token embedded in git process argv (
git/extensions/models/_lib/operations.ts:55-56)When
args.tokenis set, the token is inserted into the URL passed togit clone, making it visible in/proc/<pid>/cmdlineandps auxoutput for the lifetime of the process. Error messages, stored resources, and log lines are scrubbed correctly, but the process table exposure is unavoidable with this approach. Alternatives (GIT_ASKPASS, a transient credential helper) are significantly more complex and this pattern is widely used in CI tooling, so this is informational rather than a hard block.diffFilteris unconstrained (git/extensions/models/_lib/schemas.ts:63)The field accepts any string but git's
--diff-filteronly accepts[ACDMRTUXBacdmrtuxb*!]. An invalid value will surface as a git error (correctly propagated), but restricting with.regex(/^[ACDMRTUXBacdmrtuxb*!]+$/)would give callers an earlier, more actionable error message.git status --porcelainrename entries (git/extensions/models/_lib/operations.ts:207-210)Porcelain v1 represents renames as
R new-name\told-name(tab-separated old path appended). The currentline.substring(3)will include the tab and original path inpath, which callers may not expect. Consider either documenting this behavior or splitting on\tand taking only the first segment. Low impact since therawfield preserves the full output.Adversarial Review
Medium
scrubCredentials only matches https URLs — http credentials pass through unscrubbed
git/extensions/models/_lib/operations.ts:23
The regex only matches https:// URLs. If a caller provides an http://user:secret@host/repo URL directly (without using the token parameter), the credentials are stored unscrubbed in the resource data and log output.
Breaking example: A caller runs clone with url http://deploy:s3cr3t@internal.corp/repo. The resource written at line 81 contains the full URL with the password persisted in the resource store and emitted in the info log at line 78.
Suggested fix: Broaden the regex to cover both HTTP and HTTPS schemes, or use URL parsing for robustness.
Impact is limited because the token auth path (the main credentialed path) enforces HTTPS and is correctly scrubbed. This only affects user-provided URLs with embedded credentials, which is an uncommon but valid pattern.
runLog NUL-byte parsing has no test coverage for the inter-record newlines that real git log produces
git/extensions/models/git_test.ts:503-527
The format %H%x00%an%x00%aI%x00%s%x00 with --format= (tformat semantics) inserts a newline between each commit output. Real two-commit output includes newlines between records.
When split by NUL, the second commit SHA field becomes \nsha2. The code handles this correctly via .trim() on line 289, but the test mock data on line 506 omits the inter-record newlines entirely, so this critical parsing path is untested.
Breaking example (hypothetical): If .trim() were accidentally removed during a refactor, all tests would still pass while production parsing of multi-commit logs would produce SHAs with leading newlines. The test provides false confidence.
Suggested fix: Add a newline between records in the test mock to match real git output.
Low
AbortSignal on GitContext is accepted but never propagated
git/extensions/models/_lib/types.ts:14 / git/extensions/models/_lib/runner.ts:12
GitContext.signal exists in the interface but no operation passes it to Deno.Command. A long-running git clone of a large repository cannot be cancelled by the caller. Not a correctness bug today, but a contract that silently does nothing.
git checkout name in runBranch switch path is ambiguous with file paths
git/extensions/models/_lib/operations.ts:557
git checkout name can restore a file instead of switching branches if name matches a file but not a branch. Modern git provides git switch to avoid this ambiguity. Low risk since branch names and file names rarely collide in practice.
Token visible in process argv during git clone
git/extensions/models/_lib/operations.ts:56
The HTTPS URL with embedded token is passed as a command-line argument, making it visible in /proc/pid/cmdline on Linux for the lifetime of the clone process. This is the standard approach for CLI-based git auth and the process is short-lived, so practical risk is minimal.
Verdict
PASS — The code is well-written with strong security practices: safeRef refinements prevent flag injection on all ref-like inputs, -- separators are used correctly on positional arguments, credentials are scrubbed from error messages and resource data, and the token field is marked sensitive. The test suite is thorough with 50+ tests covering happy paths, error paths, and flag-injection prevention. The medium findings are defense-in-depth gaps, not exploitable vulnerabilities in realistic usage.