Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/harness/brain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,9 @@ impl HarnessBrain {
// over an empty ledger is a no-op, which is what every turn that touches
// no repository does.
let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts);
// Issue #735: a re-dispatched grant is not a task card, so clear any task
// a prior turn stamped — `repo_publish` requires a task and refuses here.
self.deps.checkouts.set_task(None);
// Un-streamed, like a dispatched card: this turn is answered by the
// bubble returned below, and its transient frames would otherwise
// misattribute onto whichever chat thread the console is watching.
Expand Down Expand Up @@ -633,6 +636,10 @@ impl HarnessBrain {
// guard's `Drop` is what deletes the tree on every exit — success,
// error, cancel, redirect exhaustion and panic-unwind alike.
let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts);
// Issue #735: this is a dispatched card, so `repo_publish` names its
// branch `oc/<company>/<card>`. Stamped on the same per-turn cell the
// janitor above claims.
self.deps.checkouts.set_task(Some(card.id.clone()));
// Issue #339, same argument for staged workflow references: an operator
// chat turn earlier in this cycle may have run a workflow through the
// orchestrator's tool, and that run belongs to the conversation, not to
Expand Down Expand Up @@ -2456,6 +2463,10 @@ impl HarnessBrain {
// so it can clone a repository, and the guard's `Drop`
// removes it when this turn ends.
let _checkout_janitor = CheckoutJanitor::claim(&self.deps.checkouts);
// Issue #735: a conversation is not a task card, so clear any
// task a prior turn stamped — `repo_publish` requires a task
// and refuses on a chat turn (task turns only, this tier).
self.deps.checkouts.set_task(None);
// Drive the brain-agnostic delegation seam (issue #176): the
// orchestrator turn, its queued delegations, and the CEO-relay
// hand-back all run behind the `RunTurn` impl. `HarnessDeps` is
Expand Down
170 changes: 148 additions & 22 deletions src/harness/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,8 @@ pub fn build_agent(
bindings: deps.repo_bindings.clone().into(),
workspace: workspace.clone(),
ledger: deps.checkouts.clone(),
agent: manifest_agent.id.clone(),
approvals: deps.approval_requests.clone(),
},
));
}
Expand All @@ -526,33 +528,71 @@ pub fn build_agent(
}
}

// Repository WRITE tier (issue #734). A distinct, tighter grant than the read
// `repo` above: `grants_repo_write_explicit` matches ONLY the exact
// Repository WRITE tier (issues #734, #735). A distinct, tighter grant than
// the read `repo` above: `grants_repo_write_explicit` matches ONLY the exact
// `repo.write`, so a bare `repo` (which every read-tier company writes) and
// the catch-all `*` confer nothing here — a company that asked for agents
// reading code does not silently get agents pushing it.
//
// #734 only *learns and records* whether a bound credential can push (probed
// at bind, re-probed on fetch, stored on `RepoBinding::can_push`); the
// `repo_publish` tool that consumes it lands in #735, so this wires NO tool
// yet. What it does now is the fail-closed half of #247's requirement: when
// `repo.write` is granted but no bound repository has a push-capable
// credential, say so and wire nothing, rather than leaving the operator to
// discover at publish time that the credential they bound was read-only.
// `None` (unknown — unprobed or pre-field) reads as cannot-push here, so only
// a proven `Some(true)` clears the warning.
// FOUR gates, all fail-closed, and the fourth is the one #734 added: an
// explicit `repo.write` grant, a wired manager, at least one binding, AND a
// bound credential that can actually push (`can_push == Some(true)`; `None` —
// unprobed or pre-field — reads as cannot-push). Missing any one wires
// `repo_publish` NOT AT ALL and says which, rather than offering a publish
// that would fail at push time on a read-only credential.
//
// Like the read tier, NOT feature-gated: the mirror and git runner are always
// compiled, and `repo_publish`'s push waits on an operator approval the
// runtime performs, so there is no forge client to gate the tool behind.
if crate::company::grants_repo_write_explicit(grants) {
let has_push_capable = deps
let push_capable = deps
.repo_bindings
.iter()
.any(|binding| binding.can_push == Some(true));
if !has_push_capable {
tracing::warn!(
match (&deps.repos, deps.repo_bindings.is_empty(), push_capable) {
// Gate 4 (issue #752), first and unconditional as in the read tier
// above: on a plaintext backend the credential is readable by the
// agent shell, and `repo_publish` uses it host-side to push, so no
// shape of it is safe to wire — the write path is if anything more
// exposed than the read one, never less.
(Some(repos), _, _) if repos.secrets_are_plaintext_on_disk() => tracing::warn!(
company = %company,
agent = %manifest_agent.id,
"[build] agent explicitly grants `repo.write` but this host keeps secrets on its \
own filesystem, where the credential is readable by the shell; repo_publish NOT \
wired (fail-closed, issue #752) — set OPENCOMPANY_STORAGE=mongodb or drop the \
`repo.write` grant"
),
(Some(repos), false, true) => {
tools.push(crate::harness::repo::repo_publish_tool(
crate::harness::repo::RepoToolContext {
repos: repos.clone(),
bindings: deps.repo_bindings.clone().into(),
workspace: workspace.clone(),
ledger: deps.checkouts.clone(),
agent: manifest_agent.id.clone(),
approvals: deps.approval_requests.clone(),
},
));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
(None, _, _) => tracing::warn!(
company = %company,
agent = %manifest_agent.id,
"[build] agent explicitly grants `repo.write` but no repository cache is configured \
on this host; repo_publish NOT wired (fail-closed)"
),
(Some(_), true, _) => tracing::warn!(
company = %company,
agent = %manifest_agent.id,
"[build] agent explicitly grants `repo.write` but this company has bound no \
repositories; repo_publish NOT wired (fail-closed)"
),
(Some(_), false, false) => tracing::warn!(
company = %company,
agent = %manifest_agent.id,
"[build] agent explicitly grants `repo.write` but no bound repository has a \
push-capable credential; write tools NOT wired (fail-closed)"
);
push-capable credential; repo_publish NOT wired (fail-closed)"
),
}
}

Expand Down Expand Up @@ -1638,18 +1678,53 @@ mod tests {
/// `deps.repo_bindings` — so the difference between the two is exactly
/// "the operator bound something", which is two of the four gate states.
fn built_tool_names_with_repos(grants: &[&str], bindings: usize) -> Vec<String> {
built_tool_names_with_repos_on(grants, bindings, crate::store::StorageKind::Mongodb)
built_tool_names_with_repos_full(
grants,
bindings,
false,
crate::store::StorageKind::Mongodb,
)
}

/// [`built_tool_names_with_repos`], with control over whether the bound
/// credentials read as push-capable (issue #735) — one of the two gates the
/// write tier adds beyond #245's three. `false` matches every read-tier
/// caller (`can_push: None`); the secret backend stays the safe Mongodb, so a
/// push-capability assertion never passes for the #752 reason instead.
fn built_tool_names_with_repos_cap(
grants: &[&str],
bindings: usize,
push_capable: bool,
) -> Vec<String> {
built_tool_names_with_repos_full(
grants,
bindings,
push_capable,
crate::store::StorageKind::Mongodb,
)
}

/// [`built_tool_names_with_repos`], with the secret backend spelled out —
/// the fourth gate (issue #752). Mongodb is what the plain helper passes,
/// because a host that cannot hold a repository credential safely cannot
/// reach the other three gates at all, and every assertion about them would
/// the other added gate (issue #752). Mongodb is what the plain helper
/// passes, because a host that cannot hold a repository credential safely
/// cannot reach the other gates at all, and every assertion about them would
/// otherwise be passing for the #752 reason instead of its own.
fn built_tool_names_with_repos_on(
grants: &[&str],
bindings: usize,
storage_kind: crate::store::StorageKind,
) -> Vec<String> {
built_tool_names_with_repos_full(grants, bindings, false, storage_kind)
}

/// The full repository-wiring fixture: both the push-capability (#735) and
/// the secret-backend (#752) gates spelled out. The three wrappers above each
/// default the axis they do not vary.
fn built_tool_names_with_repos_full(
grants: &[&str],
bindings: usize,
push_capable: bool,
storage_kind: crate::store::StorageKind,
) -> Vec<String> {
use crate::runtime::repo_manager::types::RepoBinding;
let dir = tempfile::tempdir().expect("tempdir");
Expand All @@ -1673,7 +1748,7 @@ mod tests {
last_fetched_millis: None,
size_bytes: 0,
bound_at_millis: 1,
can_push: None,
can_push: if push_capable { Some(true) } else { None },
})
.collect();
let manifest_agent = ManifestAgent {
Expand Down Expand Up @@ -1828,10 +1903,61 @@ mod tests {
let write_granted = built_tool_names_with_repos(&["repo.write"], 1);
assert_eq!(
write_granted, baseline,
"repo.write must wire only the read pair — no write tool exists until #735"
"repo.write with a non-push-capable credential wires only the read pair"
);
}

/// The write tier's fourth gate (issue #735): `repo_publish` is wired only
/// with `repo.write` **and** a push-capable credential, and never by the read
/// `repo` grant. The non-push-capable half is
/// `repo_write_grant_wires_no_tool_beyond_the_read_pair` above.
#[test]
fn repo_write_with_a_push_capable_credential_wires_repo_publish() {
let publish = "repo_publish".to_string();

// repo.write + a push-capable credential → repo_publish joins the belt,
// and the read pair is still there (write implies read).
let pushable = built_tool_names_with_repos_cap(&["repo.write"], 1, true);
assert!(
pushable.contains(&publish),
"a push-capable `repo.write` must wire repo_publish: {pushable:?}"
);
assert!(
pushable.contains(&"repo_checkout".to_string())
&& pushable.contains(&"repo_pr".to_string()),
"the read pair must still be wired: {pushable:?}"
);

// repo.write but a read-only credential → fail-closed, no publish.
let read_only = built_tool_names_with_repos_cap(&["repo.write"], 1, false);
assert!(
!read_only.contains(&publish),
"a read-only credential must not wire repo_publish: {read_only:?}"
);

// A bare `repo` never confers it, push-capable credential or not.
let bare = built_tool_names_with_repos_cap(&["repo"], 1, true);
assert!(
!bare.contains(&publish),
"bare `repo` (the read tier) must never wire repo_publish: {bare:?}"
);

// Gate 4 (issue #752): even a push-capable `repo.write` wires nothing on a
// plaintext secret backend — repo_publish uses the credential host-side, so
// it is refused exactly like the read tools are on such a host.
for kind in [
crate::store::StorageKind::Fs,
crate::store::StorageKind::Sqlite,
] {
let plaintext = built_tool_names_with_repos_full(&["repo.write"], 1, true, kind);
assert!(
!plaintext.contains(&publish),
"a push-capable `repo.write` on {} must not wire repo_publish: {plaintext:?}",
kind.as_str()
);
}
}

/// Granting `search` must not quietly hand over anything *else*: the
/// credentialed `["search"]` belt is the ungranted belt plus exactly one
/// tool. A namespace that widens the belt beyond its own family is how a
Expand Down
Loading
Loading