FM-403: the Git source adapter and GitHub bootstrap - #96
Merged
Merged
Conversation
The Git source provider (fleet-provider-git) clones a pinned commit into an isolated worktree named by the SHA, computes the candidate digest (commit SHA + SHA-256 of the tracked file set), and hands the YAML file set to the schemas crate for validation. Hooks never run: every git invocation passes core.hooksPath pointed nowhere (the FM-301 rule). The desired-source use cases (fleet-application/src/source.rs) gate activation: a candidate carrying diagnostics can never become active, an unknown digest was never fetched, the audit intent lands before the mutation, and a valid candidate is recorded as a rollback point while an invalid one is reported and forgotten. SourceFetch and SourceActivate join the authz catalog (36 entries) as catalog-level actions. The GitHub provider (fleet-provider-github) requests bootstrap tokens through the documented App API with least permissions (contents:read_and_write on the installation's repository only); the token is returned for Fleet's encrypted secret store and never logged or audited with its value, and GitHub's refusals are bounded and redacted. Tests: six desired-source use case tests (rollback recording, invalid candidates forgotten, activation refusals, audit+durability, denials touching nothing), plus the GitHub provider's contract shape.
source.fetch and source.activate join the executor kinds: fetch clones the pinned commit, validates through the schemas crate, and records the outcome through the authorized use case; activate re-validates against the materialized worktree (so the gate holds across restarts) and activates through the authorized use case. The source tables (active revision upsert + append-only rollback history) are a STRICT migration, and the SourceRepository adapter implements the SourcePort. The authz catalog grows to 36 entries with source.fetch/source.activate as catalog-level actions. The source dispatch is wired into the controller's executor chain.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/fleet-application/src/source.rs">
<violation number="1" location="crates/fleet-application/src/source.rs:129">
P3: `prior_revisions` includes the currently active revision. `handle_fetch` records every valid candidate into the history at fetch time, and activation later promotes that same digest to the active revision without removing it from the history. The `prior_revisions` contract promises "prior valid revisions, for manual rollback", so a manual rollback flow would present the active revision itself as a rollback target (a no-op, and misleading when the list is capped at 100 rows).</violation>
<violation number="2" location="crates/fleet-application/src/source.rs:158">
P1: Activation trusts caller-supplied validity, existence, and content digest instead of verifying the exact fetched candidate. Verify the requested digest against recorded candidate state and the materialized worktree before updating the active revision.</violation>
</file>
<file name="crates/fleet-storage-sqlite/src/source.rs">
<violation number="1" location="crates/fleet-storage-sqlite/src/source.rs:60">
P2: When more than 100 valid candidates have been fetched, this query hides older rollback points because the port has no pagination or documented retention bound. Remove the cap or add an explicit retention or pagination contract.</violation>
</file>
<file name="crates/fleet-controller/src/source.rs">
<violation number="1" location="crates/fleet-controller/src/source.rs:100">
P1: When schema input cannot be read, this fallback converts the error into zero diagnostics and records or activates the candidate as valid. Propagate the validation error or turn it into an explicit diagnostic in both fetch and activation paths.</violation>
<violation number="2" location="crates/fleet-controller/src/source.rs:161">
P1: An unvalidated `commit_sha` can escape `work_root` through this path and let activation inspect an arbitrary local directory. Require a canonical commit-SHA format and enforce that the resulting candidate path remains under the configured work root.</violation>
<violation number="3" location="crates/fleet-controller/src/source.rs:162">
P1: A directory's existence does not prove that the requested commit was fetched successfully or that its content digest matches the payload. A failed or partial clone, or an arbitrary digest supplied by the caller, can therefore be activated as the durable revision; verify the fetched commit and recomputed content digest before activating.</violation>
<violation number="4" location="crates/fleet-controller/src/source.rs:240">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
`collect_yaml` claims to return relative paths, but it ignores `_root` and pushes each absolute traversal path unchanged. The activation revalidation therefore reports worktree-prefixed paths; either make the paths relative to `root` or correct the documentation.</violation>
</file>
<file name="crates/providers/fleet-provider-git/src/lib.rs">
<violation number="1" location="crates/providers/fleet-provider-git/src/lib.rs:79">
P2: A sufficiently large repository can make `git ls-files` and Git errors consume unbounded controller memory because `Command::output` buffers both streams. Stream or size-limit Git output before accepting the candidate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| authorizer: &dyn crate::authz::Authorizer, | ||
| principal_id: &str, | ||
| digest: &fleet_core::CandidateDigest, | ||
| candidate_valid: bool, |
There was a problem hiding this comment.
P1: Activation trusts caller-supplied validity, existence, and content digest instead of verifying the exact fetched candidate. Verify the requested digest against recorded candidate state and the materialized worktree before updating the active revision.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-application/src/source.rs, line 158:
<comment>Activation trusts caller-supplied validity, existence, and content digest instead of verifying the exact fetched candidate. Verify the requested digest against recorded candidate state and the materialized worktree before updating the active revision.</comment>
<file context>
@@ -0,0 +1,449 @@
+ authorizer: &dyn crate::authz::Authorizer,
+ principal_id: &str,
+ digest: &fleet_core::CandidateDigest,
+ candidate_valid: bool,
+ candidate_known: bool,
+ ) -> Result<ActiveRevision, crate::project::ProjectUseCaseError> {
</file context>
| .extension() | ||
| .is_some_and(|extension| extension == "yaml") | ||
| { | ||
| sources.push(path); |
There was a problem hiding this comment.
P2: Custom agent: Flag AI Slop and Fabricated Changes
collect_yaml claims to return relative paths, but it ignores _root and pushes each absolute traversal path unchanged. The activation revalidation therefore reports worktree-prefixed paths; either make the paths relative to root or correct the documentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-controller/src/source.rs, line 240:
<comment>`collect_yaml` claims to return relative paths, but it ignores `_root` and pushes each absolute traversal path unchanged. The activation revalidation therefore reports worktree-prefixed paths; either make the paths relative to `root` or correct the documentation.</comment>
<file context>
@@ -0,0 +1,271 @@
+ .extension()
+ .is_some_and(|extension| extension == "yaml")
+ {
+ sources.push(path);
+ }
+ }
</file context>
Suggested change
| sources.push(path); | |
| sources.push(path.strip_prefix(_root).unwrap_or(&path).to_path_buf()); |
| } => { | ||
| if diagnostics.is_empty() { | ||
| self.port | ||
| .record_valid_revision(&ActiveRevision { |
There was a problem hiding this comment.
P3: prior_revisions includes the currently active revision. handle_fetch records every valid candidate into the history at fetch time, and activation later promotes that same digest to the active revision without removing it from the history. The prior_revisions contract promises "prior valid revisions, for manual rollback", so a manual rollback flow would present the active revision itself as a rollback target (a no-op, and misleading when the list is capped at 100 rows).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-application/src/source.rs, line 129:
<comment>`prior_revisions` includes the currently active revision. `handle_fetch` records every valid candidate into the history at fetch time, and activation later promotes that same digest to the active revision without removing it from the history. The `prior_revisions` contract promises "prior valid revisions, for manual rollback", so a manual rollback flow would present the active revision itself as a rollback target (a no-op, and misleading when the list is capped at 100 rows).</comment>
<file context>
@@ -0,0 +1,449 @@
+ } => {
+ if diagnostics.is_empty() {
+ self.port
+ .record_valid_revision(&ActiveRevision {
+ commit_sha: digest.commit_sha.clone(),
+ content_digest: digest.content_digest.clone(),
</file context>
The git provider validates the commit SHA (a full 40-character hexadecimal id, so the path cannot escape the work root), discards and recreates an existing candidate directory (a stale or partial clone is never validated under the requested SHA), refuses symlinks (the digest stays confined to the clone), bounds and redacts git output (a large repository cannot consume unbounded memory; a credential-bearing remote cannot echo its URL), and collects validation paths relative to the worktree. The GitHub provider scopes the token request to the target repository (an installation with access to more than the target must not yield a token for all of them), requests contents:write (GitHub's documented value, which grants read as well), requires the documented expiry, treats an unparseable document as a protocol-level refusal rather than a transport failure, bounds refusal details before redaction, adds schemeless credential redaction, and implements a redacted Debug that never formats the token. The desired-source use cases verify the requested digest against the recorded candidate state (only a candidate the fetch path recorded as valid and known may activate), serialize the activation critical section through the backend (a concurrent activation cannot race), correlate the audit intent with the operation id, and exclude the active revision from prior_revisions. The storage adapter implements the serialized activation with BEGIN IMMEDIATE and the source tables enforce the singleton constraint and append-only history at the database level. The executor propagates validation read failures as explicit diagnostics (never a silent valid), revalidates against the materialized worktree, and verifies the recomputed digest before activating. Tests: six new GitHub fixtures (repo scoping, missing expiry, unparseable document, oversized refusal) and the activation audit test asserts the intent's action, commit SHA, and operation correlation.
There was a problem hiding this comment.
2 existing issues remain and 3 new issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/providers/fleet-provider-github/tests/fixtures.rs">
<violation number="1" location="crates/providers/fleet-provider-github/tests/fixtures.rs:45">
P3: The tests pass the new `repository` argument but never verify it reaches the request body, so the repository-scoping property (an installation with access to more than one repo must not yield an all-repos token) is untested. The request-body assertions only check the `contents:write` permission. Add a body assertion that the repo is scoped.</violation>
</file>
<file name="crates/fleet-storage-sqlite/src/source.rs">
<violation number="1" location="crates/fleet-storage-sqlite/src/source.rs:74">
P3: Nothing calls `set_active_revision` anymore: the application's `DesiredSource::activate` (crates/fleet-application/src/source.rs) switched to `activate_serialized` (crates/fleet-application/src/source.rs:248), and a repo-wide search shows only the trait declaration, the test fake, and this SQLite impl remain. The SQLite implementation now duplicates the exact upsert of `activate_serialized` while keeping a slightly different contract (no transaction, no returned revision), so the two can drift. Either drop `set_active_revision` from `SourcePort` (with the fake and this impl) or delegate it to `activate_serialized` to keep a single write path.</violation>
</file>
<file name="crates/providers/fleet-provider-git/src/lib.rs">
<violation number="1" location="crates/providers/fleet-provider-git/src/lib.rs:118">
P2: The SHA predicate rejects uppercase hex letters A–F. Because `&&` binds tighter than `||`, the expression accepts only 0–9 and a–f; a valid 40-character hex SHA containing A–F (which git itself resolves, verified with `git rev-parse` on the uppercased full SHA) is refused with the misleading error "must be a full 40-character hexadecimal id". The commit SHA comes from the user-supplied `FetchPayload`, so callers passing an uppercase SHA fail at fetch. Accept all hex digits, or lowercase the SHA before validating/storing.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
| if commit_sha.len() != 40 | ||
| || !commit_sha | ||
| .chars() | ||
| .all(|c| c.is_ascii_hexdigit() && c.is_ascii_lowercase() || c.is_ascii_digit()) |
There was a problem hiding this comment.
P2: The SHA predicate rejects uppercase hex letters A–F. Because && binds tighter than ||, the expression accepts only 0–9 and a–f; a valid 40-character hex SHA containing A–F (which git itself resolves, verified with git rev-parse on the uppercased full SHA) is refused with the misleading error "must be a full 40-character hexadecimal id". The commit SHA comes from the user-supplied FetchPayload, so callers passing an uppercase SHA fail at fetch. Accept all hex digits, or lowercase the SHA before validating/storing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/providers/fleet-provider-git/src/lib.rs, line 118:
<comment>The SHA predicate rejects uppercase hex letters A–F. Because `&&` binds tighter than `||`, the expression accepts only 0–9 and a–f; a valid 40-character hex SHA containing A–F (which git itself resolves, verified with `git rev-parse` on the uppercased full SHA) is refused with the misleading error "must be a full 40-character hexadecimal id". The commit SHA comes from the user-supplied `FetchPayload`, so callers passing an uppercase SHA fail at fetch. Accept all hex digits, or lowercase the SHA before validating/storing.</comment>
<file context>
@@ -103,33 +110,49 @@ impl GitSource {
+ if commit_sha.len() != 40
+ || !commit_sha
+ .chars()
+ .all(|c| c.is_ascii_hexdigit() && c.is_ascii_lowercase() || c.is_ascii_digit())
+ {
+ return Err("the commit SHA must be a full 40-character hexadecimal id".to_owned());
</file context>
Suggested change
| .all(|c| c.is_ascii_hexdigit() && c.is_ascii_lowercase() || c.is_ascii_digit()) | |
| .all(|c| c.is_ascii_hexdigit()) |
| seen: seen.clone(), | ||
| }; | ||
| let outcome = | ||
| request_bootstrap_token(&transport, "12345", "example/repo", Duration::from_secs(30)) |
There was a problem hiding this comment.
P3: The tests pass the new repository argument but never verify it reaches the request body, so the repository-scoping property (an installation with access to more than one repo must not yield an all-repos token) is untested. The request-body assertions only check the contents:write permission. Add a body assertion that the repo is scoped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/providers/fleet-provider-github/tests/fixtures.rs, line 45:
<comment>The tests pass the new `repository` argument but never verify it reaches the request body, so the repository-scoping property (an installation with access to more than one repo must not yield an all-repos token) is untested. The request-body assertions only check the `contents:write` permission. Add a body assertion that the repo is scoped.</comment>
<file context>
@@ -41,9 +41,10 @@ async fn the_token_request_carries_least_permissions() {
- .await
- .unwrap();
+ let outcome =
+ request_bootstrap_token(&transport, "12345", "example/repo", Duration::from_secs(30))
+ .await
+ .unwrap();
</file context>
| .collect()) | ||
| } | ||
|
|
||
| async fn activate_serialized( |
There was a problem hiding this comment.
P3: Nothing calls set_active_revision anymore: the application's DesiredSource::activate (crates/fleet-application/src/source.rs) switched to activate_serialized (crates/fleet-application/src/source.rs:248), and a repo-wide search shows only the trait declaration, the test fake, and this SQLite impl remain. The SQLite implementation now duplicates the exact upsert of activate_serialized while keeping a slightly different contract (no transaction, no returned revision), so the two can drift. Either drop set_active_revision from SourcePort (with the fake and this impl) or delegate it to activate_serialized to keep a single write path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/fleet-storage-sqlite/src/source.rs, line 74:
<comment>Nothing calls `set_active_revision` anymore: the application's `DesiredSource::activate` (crates/fleet-application/src/source.rs) switched to `activate_serialized` (crates/fleet-application/src/source.rs:248), and a repo-wide search shows only the trait declaration, the test fake, and this SQLite impl remain. The SQLite implementation now duplicates the exact upsert of `activate_serialized` while keeping a slightly different contract (no transaction, no returned revision), so the two can drift. Either drop `set_active_revision` from `SourcePort` (with the fake and this impl) or delegate it to `activate_serialized` to keep a single write path.</comment>
<file context>
@@ -71,13 +71,48 @@ impl fleet_application::source::SourcePort for SourceRepository {
.collect())
}
+ async fn activate_serialized(
+ &self,
+ revision: &ActiveRevision,
</file context>
The git provider accepts uppercase hex SHAs (lowercased, matching git's own resolution) and refuses a tracked-file listing beyond its 1 MiB bound — a truncated listing would hash only part of the file set, so truncation is an error rather than an incomplete digest. The GitHub fixtures assert the repository scoping reaches the request body. The source tables enforce their invariants at the database level: the active revision's primary key is the singleton id, and the rollback history is append-only (BEFORE UPDATE/DELETE triggers, matching the audit ledger). The history key carries both the commit SHA and the content digest, so two commits with the same tracked-file digest are both preserved. The source kinds' permission is enforced during operation creation with resource: None (catalog-level, like the dedicated endpoint), and the unused set_active_revision port method is removed — activate_serialized is the single write path.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/providers/fleet-provider-ssh/tests/trust.rs">
<violation number="1" location="crates/providers/fleet-provider-ssh/tests/trust.rs:45">
P3: This lock only serializes sshd startup across threads within the `trust` test binary; it does not protect the free-port/bind window when test binaries run concurrently. The sibling suite `tests/exec.rs` already solves this same race with a uid-scoped cross-process file lock (`acquire_startup_lock`), whose comment explicitly covers "across threads and across concurrently run test binaries." If `trust` and `exec` ever run at the same time (parallel test runners, nextest, sharded/concurrent test invocations on a shared host), the invalid config fires.
Reuse the existing cross-process lock (extract a shared helper, or guard via the same `fleet-test-sshd-startup-{user}.lock` file) instead of introducing a second, weaker serialization mechanism for the same resource.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| /// current user, authenticating via the agent or an unencrypted key we also | ||
| /// generate here. | ||
| fn start_sshd() -> TestSshd { | ||
| let _guard = STARTUP_LOCK |
There was a problem hiding this comment.
P3: This lock only serializes sshd startup across threads within the trust test binary; it does not protect the free-port/bind window when test binaries run concurrently. The sibling suite tests/exec.rs already solves this same race with a uid-scoped cross-process file lock (acquire_startup_lock), whose comment explicitly covers "across threads and across concurrently run test binaries." If trust and exec ever run at the same time (parallel test runners, nextest, sharded/concurrent test invocations on a shared host), the invalid config fires.
Reuse the existing cross-process lock (extract a shared helper, or guard via the same fleet-test-sshd-startup-{user}.lock file) instead of introducing a second, weaker serialization mechanism for the same resource.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/providers/fleet-provider-ssh/tests/trust.rs, line 45:
<comment>This lock only serializes sshd startup across threads within the `trust` test binary; it does not protect the free-port/bind window when test binaries run concurrently. The sibling suite `tests/exec.rs` already solves this same race with a uid-scoped cross-process file lock (`acquire_startup_lock`), whose comment explicitly covers "across threads and across concurrently run test binaries." If `trust` and `exec` ever run at the same time (parallel test runners, nextest, sharded/concurrent test invocations on a shared host), the invalid config fires.
Reuse the existing cross-process lock (extract a shared helper, or guard via the same `fleet-test-sshd-startup-{user}.lock` file) instead of introducing a second, weaker serialization mechanism for the same resource.</comment>
<file context>
@@ -38,6 +42,9 @@ fn free_port() -> u16 {
/// current user, authenticating via the agent or an unencrypted key we also
/// generate here.
fn start_sshd() -> TestSshd {
+ let _guard = STARTUP_LOCK
+ .lock()
+ .unwrap_or_else(std::sync::PoisonError::into_inner);
</file context>
This was referenced Sep 19, 2026
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
1 participant
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.
Closes #92 (FM-403). Part of epic #8 — completes M4.
What
fleet-provider-git, new crate): an isolated clone/worktree per candidate named by the SHA, the candidate digest (commit SHA + SHA-256 of the tracked file set), and hooks that never run (core.hooksPathpointed nowhere on every invocation — the FM-301 rule). The YAML file set is handed to the schemas crate for validation.fleet-application/src/source.rs): a valid candidate is recorded as a rollback point; a candidate carrying diagnostics is reported and forgotten; activation refuses invalid and unknown candidates; the audit intent lands before the mutation; prior valid revisions are available for manual rollback.SourceFetch/SourceActivatejoin the authz catalog (36 entries) as catalog-level actions.source.fetch,source.activate): durable operations; activate re-validates against the materialized worktree so the gate holds across restarts.SourceRepositoryadapter.fleet-provider-github): the App installation-token flow with least permissions (contents:read_and_writeon the installation's repository only); the token is returned for Fleet's encrypted secret store, never logged or audited with its value; refusals are bounded and redacted.Tests
cargo xtask verifypasses.Summary by cubic
Completes FM-403 by making a Git repository the canonical desired-state source and replacing the GitHub provider skeleton with the App bootstrap flow. Fleet can now fetch a pinned commit, validate its YAML, and activate it as the desired revision, with valid candidates kept as manual rollback points.
New Features
fleet-provider-gitclones each pinned commit into an isolated worktree named by the SHA, digests the tracked file set with SHA-256, refuses symlinks and tracked-file listings beyond 1 MiB, and disables git hooks viacore.hooksPath.source.fetchandsource.activateare durable executor kinds; activation re-validates the materialized worktree and writes the audit intent before mutating state.SourceFetchandSourceActivatejoin the authz catalog as catalog-level actions enforced at operation creation.contents:writeon the target repository only, requires a documented expiry, returns the token for the encrypted secret store, and redacts refusals without logging the token.Migration
source_active_revision(single-row upsert) andsource_revision_history(append-only), backed bySourceRepository.Written for commit e79de99. Summary will update on new commits.