fix(repo): write-tier reliability — commit gpg-hang, push 300s hang, silent push failure (#815) - #817
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCheckout retention now preserves workspaces across approval turns and reuses matching refs. Git publishing uses private file-backed askpass credentials. Publish failures create classified notes on real task cards. Lossy DM keys receive stable digests. ChangesCheckout lifecycle
Publishing reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🔵 Low · up to The change adds an operator-facing note and rerun guidance when publishing fails, but the write-tier specification does not yet document that behavior. The PR is mergeable with explicit owner follow-up to update the specification. Sequence Diagram(s)sequenceDiagram
participant ApprovalWorkflow
participant CheckoutLedger
participant repo_checkout
ApprovalWorkflow->>CheckoutLedger: retain_for_task(task)
ApprovalWorkflow->>CheckoutLedger: reclaim(task)
ApprovalWorkflow->>repo_checkout: request checkout ref
repo_checkout->>CheckoutLedger: has_active(path)
repo_checkout-->>ApprovalWorkflow: reuse matching checkout
sequenceDiagram
participant PublishCycle
participant AskpassDir
participant Git
participant TaskCard
PublishCycle->>AskpassDir: write_token
PublishCycle->>Git: spawn with GIT_ASKPASS
Git->>AskpassDir: read token for each prompt
PublishCycle->>TaskCard: append classified failure note
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
cluster:spine
24138ac to
4dad61a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/harness/repo.rs (1)
857-881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a tool-level checkout reuse test.
Invoke
RepoCheckoutTool::executetwice and confirm that the second call preserves changes in the existing checkout instead of re-materializing it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/harness/repo.rs` around lines 857 - 881, Add a tool-level test around RepoCheckoutTool::execute that performs two checkout calls for the same task and destination, modifies the checkout after the first call, then verifies the second call reuses the existing directory and preserves the modification instead of re-materializing it.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cycle.rs`:
- Around line 929-956: Make conversation-derived work keys collision-resistant
in approval_work_key and the corresponding thread-key path around the other
affected usage. Compute a short stable hash from the original unsanitized
thread, then incorporate it into the sanitized segment so separator variants
remain distinct while retaining a safe branch segment. Leave task-card IDs
unchanged and ensure the same derived value is used by origin_task,
CheckoutLedger, and repo_publish.
In `@src/runtime/repo_manager/git.rs`:
- Around line 193-214: Harden the askpass setup around the directory-creation
logic and write_token: generate the directory name with getrandom::fill, create
it exclusively with create_dir, and replace std::fs::write with
OpenOptions::create_new(true), applying mode 0600 before writing the token. Add
Unix-specific tests covering exclusive directory creation, token symlink
rejection, and the resulting file permissions.
---
Nitpick comments:
In `@src/harness/repo.rs`:
- Around line 857-881: Add a tool-level test around RepoCheckoutTool::execute
that performs two checkout calls for the same task and destination, modifies the
checkout after the first call, then verifies the second call reuses the existing
directory and preserves the modification instead of re-materializing it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ccfae58-a57b-402d-886f-a1e707d05eb0
📒 Files selected for processing (11)
docs/spec/runtime/repos.mdsrc/harness/brain.rssrc/harness/policy.rssrc/harness/repo.rssrc/harness/repo/test.rssrc/policy/consequence.rssrc/runtime/cycle.rssrc/runtime/grants.rssrc/runtime/journal.rssrc/runtime/repo_manager/git.rssrc/server/operator.rs
…ai#815) An agent's checkout inherits the host operator's global git config, so on a host with `commit.gpgsign = true` every `git commit` blocks on a GPG key the sandbox cannot reach — the change stages but never commits and the write flow stalls with nothing published. `attribute_checkout` now forces `commit.gpgsign` / `tag.gpgsign` to `false` alongside the agent-seat identity; per-agent commit signing is the deferred issue tinyhumansai#738. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ing (tinyhumansai#815) git invokes the askpass helper once per authenticated request, and a `git push` authenticates TWICE (ref-advertisement probe + pack upload). The token was fed on the child's stdin — a one-shot pipe — so the second prompt read EOF and, with the OS keychain deliberately disabled here, `git-remote-https` stalled to the 300s network timeout instead of failing. The write tier's push hung for exactly this reason; a fetch (one auth) never showed it, and any hand-test fell back to the OS keychain and masked it. Write the token into a 0600 file in the helper's private `$HOME` instead, read on every prompt. Still never in argv, environment, git config or a URL — only briefly in a per-invocation file removed on drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sai#815) A failed host-side push leaves the effect recorded as executed (the at-most-once guard), so re-approving is a no-op. With no operator-facing note the change sits staged in the mirror, nothing reaches the remote, the approval is gone, and the operator believes it published. Post a task board note on push failure so they know nothing landed and the way back is to re-run the task. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…inyhumansai#815) Two review findings on the write-tier reliability change: - The askpass helper's `$HOME` was a predictable `.askpass-<pid>-<seq>` made with `create_dir_all`, and the token was written with `std::fs::write` — which follows a pre-planted `token` symlink before the 0600 mode lands. A local attacker sharing the repo cache could pre-create either and capture the PAT. The directory name is now 16 bytes from the OS CSPRNG created with an exclusive, non-recursive `create_dir` (0700 at creation), and both the script and the token are written with `create_new` at their mode — never following a symlink, never adopting an existing file. - `sanitize_work_segment` folds every disallowed character to `-`, itself a keep-character, so `coder/main` and `coder-main` reduced to the same DM work key — colliding checkout retention and the publish branch. When folding or trimming loses information it now appends a short build-stable digest of the raw thread, so distinct threads keep distinct keys; an already-safe thread is unchanged and stays readable. Tests: unix coverage for exclusive dir/token creation, symlink rejection, and 0600 mode; a sanitize collision + determinism case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4dad61a to
5888969
Compare
|
@coderabbitai on the nitpick — a tool-level test for the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/harness/repo.rs --items all --type function,method --match 'a_second_checkout_at_a_different_ref_is_refused_not_silently_reused|execute' || true
rg -n -A90 -B20 \
'a_second_checkout_at_a_different_ref_is_refused_not_silently_reused|fn execute\s*\(' \
src/harness/repo.rs
printf '\n--- relevant diff history ---\n'
git show --stat --oneline 500b0ebf -- src/harness/repo.rs 2>&1 || true
git show --format= --find-renames 500b0ebf -- src/harness/repo.rs 2>&1 | sed -n '1,260p' || trueLength of output: 25208 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -A180 -B30 \
'a_second_checkout_at_a_different_ref_is_refused_not_silently_reused' \
.
printf '\n--- test module map ---\n'
fd -t f 'test\.rs$|repo.*test.*\.rs$|.*repo.*\.rs$' src/harness | sort
printf '\n--- direct RepoCheckoutTool::execute calls in harness tests ---\n'
rg -n -A8 -B8 \
'RepoCheckoutTool|\.execute\(json!\(\{.*"repo"' \
src/harness -g '*.rs'Length of output: 36271
The test covers same-ref reuse, different-branch refusal, and PR-over-branch refusal. It also verifies the ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/spec/runtime/repos.md (1)
482-494: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument host-side push failures.
The
repo_publishdescription documents task reporting when pull-request creation fails, but it does not document the new host-side push-failure contract. Add that a failed push posts a task discussion, states that no commit reached the remote, and tells the operator to rerun the task.Proposed documentation update
- `repo_publish` (issue `#735`). The agent commits locally in its checkout, then publishes host-side. The host *fetches* the checkout's committed HEAD into the mirror on a host-owned `oc/<company>/<task>` branch — a fetch never invokes `receive-pack`, so the read tier's `pre-receive` refusal and its no-push contract test stay untouched — and, only after the operator approves, pushes that exact commit to the remote. The agent still holds no credentialed remote and never pushes; every structural refusal (host-generated namespaced branch, never a force push, never the default branch, never a ref outside `oc/`) lives in `RepoManager` where no prompt can reach it. + If the host-side push fails, `repo_publish` posts a task discussion stating + that no commit reached the remote and that the task must be rerun.This matches the PR objective for push-failure task reporting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/spec/runtime/repos.md` around lines 482 - 494, Update the repo_publish documentation to describe host-side push failures: the task must post a discussion stating that no commit reached the remote and instructing the operator to rerun the task. Keep the existing pull-request failure behavior unchanged.
🧹 Nitpick comments (1)
src/runtime/grants.rs (1)
528-578: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the pending window in this module.
mark_pending,clear_pending, and the third arm ofany_for_taskcarry the whole#796guarantee, and this module'smod testcovers none of them. The existing fixtures were only updated withorigin_task: None.A small test pins the three transitions that matter: a pending mark alone makes
any_for_tasktrue,clear_pendingmakes it false, and a second approval on the same task clears independently.The repository guidelines require focused tests with every behavior change and keep package-local tests in the module file.
🧪 Suggested test
/// Issue `#796`: the window between a park and the operator's decision. #[test] fn a_still_parked_approval_keeps_its_task_alive() { let set = GrantSet::default(); assert!(!set.any_for_task("t-1")); set.mark_pending(&ApprovalId::new("a1"), "t-1".to_string()); assert!( set.any_for_task("t-1"), "a parked approval mints no grant, so only the mark can hold the checkout" ); // A second step of the same task, cleared independently. set.mark_pending(&ApprovalId::new("a2"), "t-1".to_string()); set.clear_pending(&ApprovalId::new("a1")); assert!(set.any_for_task("t-1"), "the task's other step is still parked"); set.clear_pending(&ApprovalId::new("a2")); assert!(!set.any_for_task("t-1"), "nothing names it now, so it is sweepable"); // A live grant is the other arm, and it must not be confused with the mark. set.grant(GrantedCall { origin_task: Some("t-2".to_string()), ..call("a3", "ops", "shell", serde_json::json!({})) }); assert!(set.any_for_task("t-2")); assert!(set.consume("ops", "shell", &serde_json::json!({})).is_some()); assert!(!set.any_for_task("t-2"), "a spent grant names nothing"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/grants.rs` around lines 528 - 578, Add a focused unit test in the module’s existing mod test for the pending-approval lifecycle, using mark_pending, clear_pending, and any_for_task: verify one pending approval keeps its task alive, independently clearing one of two approvals leaves it alive, clearing both makes it false, and preserve the existing live-grant/consume behavior to confirm spent grants no longer keep a task alive.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cycle.rs`:
- Around line 1338-1362: Update both repository publish failure branches around
repos.push_published so TaskDiscussionPosted is emitted only after resolving
payload["task"] to an existing card ID, rather than using the resumed DM work
key from task. When the task is a dm-* key or no card resolves, report the
failure through the existing operator channel instead of appending a discussion
to a nonexistent card, while preserving the returned publish error.
---
Outside diff comments:
In `@docs/spec/runtime/repos.md`:
- Around line 482-494: Update the repo_publish documentation to describe
host-side push failures: the task must post a discussion stating that no commit
reached the remote and instructing the operator to rerun the task. Keep the
existing pull-request failure behavior unchanged.
---
Nitpick comments:
In `@src/runtime/grants.rs`:
- Around line 528-578: Add a focused unit test in the module’s existing mod test
for the pending-approval lifecycle, using mark_pending, clear_pending, and
any_for_task: verify one pending approval keeps its task alive, independently
clearing one of two approvals leaves it alive, clearing both makes it false, and
preserve the existing live-grant/consume behavior to confirm spent grants no
longer keep a task alive.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0abe5a93-44fa-4156-81b2-392b7a3bb656
📒 Files selected for processing (11)
docs/spec/runtime/repos.mdsrc/company/runtime.rssrc/harness/brain.rssrc/harness/repo.rssrc/harness/repo/test.rssrc/policy/consequence.rssrc/runtime/cycle.rssrc/runtime/grants.rssrc/runtime/journal.rssrc/runtime/repo_manager/git.rssrc/server/operator.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/harness/repo/test.rs
- src/policy/consequence.rs
- src/runtime/journal.rs
- src/server/operator.rs
- src/harness/brain.rs
- src/harness/repo.rs
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
All three diagnoses are excellent, and the second is the best piece of debugging I have read in this repo:
git pushauthenticates twice (ref-advertisement probe + pack upload). The second askpass invocation read EOF, and with the OS keychain deliberately disabled …git-remote-httpsstalled to the timeout instead of failing. … A fetch authenticates once, which is why only push hung — and why every hand-test masked it via the OS keychain.
That last clause is the whole reason this survived #735's review, including mine. A one-shot stdin pipe answering a prompt that fires twice is invisible to anyone whose machine has a keychain to fall back on.
The token handling in the fix is careful, and I checked it rather than taking it: the helper script itself carries no credential (so nothing sensitive lands in ps or in the script on disk), the token is a 0600 file in a per-operation scratch $HOME that is removed when dropped, HOME is redirected so the operator's own .gitconfig cannot be resolved, and GIT_TERMINAL_PROMPT=0 with no askpass installed is the fail-closed path when there is no credential at all.
Forcing commit.gpgsign/tag.gpgsign to false in attribute_checkout is right: an agent's host-side commit has no business being signed with the operator's key, and inheriting a global that expects pinentry in a sandbox is a hang rather than a failure. Deferring per-agent signing to #738 rather than improvising it here is the correct scope.
1 major. Requesting changes — the fix is one line and I would rather it ship correct than ship twice.
Major — a raw subprocess error goes into a durable, operator-facing record
let note = format!(
"Could not publish `{branch}` to the remote: {err}. Nothing reached the \
remote, and this approval will not retry on its own — re-run the task to \
publish again."
);{err} is the unfiltered error from a git child process, and this note is posted as a TaskDiscussionPosted — durable, and readable by any company member.
Git's failure text routinely carries the remote URL and local filesystem paths: the scratch $HOME the askpass helper lives in, and the checkout path. That is precisely what this codebase has already decided not to do, in the same subsystem, one PR over — #688's PayloadStorage::Refused deliberately excludes the store's error text from the artifact body, with the reason written at the site: "this string is permanent, and a backend error can name host paths; the operator needs to know the file is not there, and the diagnosis belongs in the log." #614 is the older precedent, where a URL's userinfo reached the durable approval journal.
I do not think the token is reachable here — it travels through askpass and never appears in a URL, which is exactly why that design is good. So this is host paths and remote URLs rather than a credential leak, which is why I am flagging it as Major and not Critical.
The fix is the shape #688 already established: keep tracing::warn!(branch, "...: {err}") exactly as it is — the diagnosis belongs in the log — and give the note a classified sentence instead of the raw text. The operator needs three facts, none of which require {err}: the branch did not reach the remote, the approval will not retry, and re-running the task is the way back. All three are already in your sentence.
If the team wants the two hang fixes on main today, this does not have to hold them up — swapping {err} for a fixed clause is a one-line change, not a redesign.
Also worth fixing before merge
The description's SHAs are stale. It lists b15c5da5 as the push-failure commit; the branch has 24138ac4. Harmless in itself, but this repo has lost review fixes twice this week to a merge landing against a head someone believed was current, so a description that names commits which are not on the branch is worth correcting rather than leaving.
144 commits behind, and stacked on #803 which is not on main. The stacking is stated clearly, which is the right way to present it — but the net change here wants re-verifying against main once #803 lands, not just against its current parent.
Answered in the code, and I want to note it approvingly
I went looking for whether "re-run the task" is actually possible given the at-most-once guard, and the comment answers it before the question can be asked:
A failed push leaves the effect recorded as executed (the at-most-once guard), so re-approving is a no-op; the operator has to KNOW it failed, and that the way back is to re-run the task, or the publish vanishes with the change staged in the mirror and nothing on the remote.
Stating the recovery path and why the obvious one does not work is what makes the note trustworthy rather than reassuring.
…ds (tinyhumansai#815) Two review findings on the publish-failure board notes: - The note embedded the raw `git` error, which routinely names the remote URL and host paths (the askpass scratch $HOME, the checkout) — in a record that is durable and readable by any company member. It now carries a fixed, classified sentence (the branch did not reach the remote, the approval will not retry, re-run the task); the diagnosis stays in the `tracing::warn`, where a host-path-bearing error belongs (the rule tinyhumansai#614/tinyhumansai#688 set). Applies to both the push-failure and the PR-open-failure note. - A DM's `repo_publish` stamps its `dm-*` work key as the effect's `task`, which no card owns, so `TaskDiscussionPosted` filed the note against a phantom card. The note is now posted only when the id resolves to a real card; otherwise the failure stays in the operator log. Test: `task_names_a_card` matches a real card and rejects a `dm-*` key, an unknown id, and the empty id. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mark_pending/clear_pending and the pending arm of any_for_task carry the tinyhumansai#796 guarantee but had no direct unit coverage in this module — the sweep test in harness::repo exercised them only through the ledger. Pin the three transitions: a pending mark alone keeps a task live, clearing it drops it, and two approvals on one task clear independently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai on the two summary items:
|
|
@oxoxDev — thank you, and all three points are addressed. Major — raw Stale SHAs in the description. Corrected — I rewrote the description to describe the changes by content rather than pin SHAs that a rebase invalidates (the exact failure you flagged), and added the review-hardening section. 144 behind / stacked on #803. #803 (#796) merged at 05:20 today, so this is now the write-tier reliability change on its own. I've merged current Appreciate the careful read on the token handling — that was the part I most wanted a second set of eyes on. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance mentioning the documentation cap ---'
rg -n -C2 '500.?line|500 lines|repos\.md' CLAUDE.md 2>/dev/null || true
printf '%s\n' '--- target file line count ---'
wc -l docs/spec/runtime/repos.md
printf '%s\n' '--- GrantSet and pending-window test map ---'
ast-grep outline src/runtime/grants.rs --items all --type function,method --match 'any_for_task|mark_pending|clear_pending|a_still_parked_approval_keeps_its_task_alive' --view expanded || true
printf '%s\n' '--- pending-window test and GrantSet implementation ---'
rg -n -A130 -B25 \
'a_still_parked_approval_keeps_its_task_alive|fn (mark_pending|clear_pending|any_for_task)\b' \
src/runtime/grants.rs
printf '%s\n' '--- push-failure code documentation and classified note text ---'
rg -n -A75 -B25 \
'note_publish_failure_on_card|nothing reached the remote|approval will not retry|re-run the task' \
src
printf '%s\n' '--- claimed test commit ---'
git show --format=fuller --stat 47fe3acf -- src/runtime/grants.rs 2>&1 || trueLength of output: 30370
🐇 ✏️ Learnings added
You are interacting with an AI system. |
coderabbitai
Bot
added
the
cluster:approvals
…ai#815) The exclusivity test asserted the random dir name does not *contain* the pid as a substring — flaky, since a 32-char hex string can hold the pid's decimal digits by chance (CI hit exactly that: .askpass-db1ab4f4...). Pin the real property instead: a full-width hex suffix, and a second create landing on a different name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0439 · 47,890 in / 12,586 out · 35,745 cached (75%) · z-ai/glm-5.2
critique: $0.0143 · 9,080 in / 4,858 out · 6,509 cached (72%) · z-ai/glm-5.2
security: $0.0094 · 9,059 in / 2,731 out · 6,388 cached (71%) · z-ai/glm-5.2
tests: $0.0110 · 14,308 in / 3,036 out · 11,431 cached (80%) · z-ai/glm-5.2
description: $0.0093 · 15,443 in / 1,961 out · 11,417 cached (74%) · z-ai/glm-5.2
What this change touches5 files, +513 -134 across 4 components. It reaches 6 untouched components (60 graph nodes walked). flowchart LR
n0["src/runtime/repo_manager<br/>1 file +265 -101"]:::changed
n1["src/runtime<br/>2 files +211 -32"]:::changed
n2["src/harness/repo<br/>1 file +24 -0"]:::changed
n3["src/harness<br/>1 file +13 -1"]:::changed
n4["src/runtime<br/>4 files reached"]:::impacted
n5["src/ports<br/>3 files reached"]:::impacted
n6["src/company<br/>2 files reached"]:::impacted
n7["src<br/>1 file reached"]:::impacted
n8["src/harness<br/>1 file reached"]:::impacted
n9["src/server<br/>1 file reached"]:::impacted
n4 -->|34 refs| n5
n4 -->|12 refs| n6
n4 -->|8 refs| n7
n6 -->|5 refs| n4
n6 -->|3 refs| n5
n5 -->|2 refs| n7
n6 -->|2 refs| n7
n8 -->|2 refs| n5
n9 -->|2 refs| n4
n9 -->|2 refs| n5
n9 -->|2 refs| n6
n5 -->|1 ref| n6
n7 -->|1 ref| n4
n7 -->|1 ref| n5
n7 -->|1 ref| n6
n8 -->|1 ref| n4
n8 -->|1 ref| n6
n9 -->|1 ref| n7
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
priority: p3
2 participants
Summary
Three write-tier reliability bugs, each of which independently stops an agent from publishing — all surfaced while validating #796 in the live console, and all confirmed fixed end to end (a real PR opened once the three landed). Fixed together per the tracking issue.
commit.gpgsign = true, so everygit_operationscommit tried to GPG-sign and hung (no pinentry in the sandbox); the change staged but never committed.attribute_checkoutnow forcescommit.gpgsign/tag.gpgsigntofalse(per-agent commit signing is the deferred feat(repo): sign agent commits with a per-agent identity key (deferred from #247) #738).git::runfed the PAT to the askpass on the child's one-shot stdin pipe, but agit pushauthenticates twice (ref-advertisement probe + pack upload). The second askpass invocation read EOF, and with the OS keychain deliberately disabled (GIT_CONFIG_NOSYSTEM,credential.helper=cleared)git-remote-httpsstalled to the timeout instead of failing. The askpass now reads the token from a 0600 file in its private$HOME, re-read on every prompt. (A fetch authenticates once, which is why only push hung — and why every hand-test masked it via the OS keychain.)perform_effectnow posts a task note on push failure ("nothing reached the remote — re-run the task").Review hardening
Fixes from the review of the three above:
$HOMEwas a predictable.askpass-<pid>-<seq>made withcreate_dir_all, and the token was written withstd::fs::write, which follows a pre-plantedtokensymlink before0600lands. The directory name is now 16 bytes from the OS CSPRNG created with an exclusivecreate_dir(0700 at creation), and the script and token are written withcreate_newat their mode — never following a symlink, never adopting an existing file.sanitize_work_segmentfolded every disallowed character to-(a keep-character), socoder/mainandcoder-maincollided onto one work key. It now appends a build-stable digest of the raw thread when folding/trimming loses information; already-safe threads stay readable.giterror (which can name host paths and the remote URL) in a durable, company-readable record; it now carries a fixed, classified sentence with the diagnosis kept to the log (workflows: an http_request node never consults ApprovalPolicy, so its requests raise no operator approval #614/publish: tell the truth about a stored payload, and record its digest #688). A DM'srepo_publishstamps adm-*work key that no card owns, so the note is now posted only when the id resolves to a real card.Base
Was stacked on #796; #796 (#803) has since merged, so this branch is now the write-tier reliability change on its own, merged up to current
main.Test plan
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warnings(default)cargo clippy --features openhuman,tinycortex --all-targets -- -D warningssanitize_work_segmentcollision + determinism;GrantSetpending-approval window; a publish note lands only on a real card, not adm-*keyruntime::repo_manager::git+harness::repo+runtime::grants+runtime::cyclemodule tests passrepo_checkout → edit → commit → publish → push → PRin the consoleCloses #815.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes