Skip to content

fix(go): gate resolver permission-mode to claude, truthful push reporting, test-before-push prompt - #118

Open
AbirAbbas wants to merge 3 commits into
mainfrom
fix/agent-reliability
Open

fix(go): gate resolver permission-mode to claude, truthful push reporting, test-before-push prompt#118
AbirAbbas wants to merge 3 commits into
mainfrom
fix/agent-reliability

Conversation

@AbirAbbas

@AbirAbbas AbirAbbas commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Rebuilt on top of current main after a deep review of the original branch. Three fixes land here; a fourth was dropped (see below).

1. Gate the resolver's writable permission mode to the claude runtime

run_pr_resolver was handed cfg.PermissionMode verbatim, which defaults to "". Under the claude harness an empty permission mode omits --permission-mode entirely, so claude --print falls back to its interactive prompting default — with nobody to answer the prompt, every write is denied and the resolver finishes having produced no commits.

The default is now "auto" (→ bypassPermissions), but only when the resolved provider is claude. Checked against sdk/go/harness/:

provider empty mode "auto" verdict
claude no flag → CLI prompts → read-only under -p bypassPermissions fix is real
codex --sandbox workspace-write — already writable --dangerously-bypass-approvals-and-sandbox "auto" would be harmful: drops the sandbox around the whole machine for no benefit
opencode PermissionMode is never read never read no-op; leave empty rather than imply a guarantee we do not make

An explicitly configured permission mode is still passed through unchanged for every provider.

2. Truthful push reporting

When the harness returns nothing parseable, run_pr_resolver hands back a deterministic all-false PRResolveResult. The orchestrator took that at face value — so a run where the agent committed and pushed but failed to emit a final structured answer was reported as "nothing happened", and the CI gate was skipped because pushed was false.

The head branch is now sampled on the remote (git ls-remote, since the agent pushes from its own process and leaves this workspace's tracking refs stale) before and after the agent runs, and only what that comparison can actually prove is recorded:

  • report_invalid: true is set whenever the sentinel report is seen, independently of the remote, so callers can never mistake a reconstructed result for a first-hand one.
  • Remote moved and the new tip == this workspace's HEADpushed=true. fixed stays false: a landed commit is not evidence that CI passes or that review comments were addressed. Overall success is fixed && pushed, so it correctly stays false.
  • Remote moved to some other commit (a third party pushed while we ran) → remote_advanced: true and nothing is attributed. Their push is not ours to claim and their commits are not ours to describe.
  • Remote did not move → nothing is claimed at all.

Commit/file lists are reconstructed only after git fetch origin <branch> makes the post-push objects resolvable locally. If that fetch fails, the lists stay empty and verification_partial: true marks them as unknown rather than as "nothing changed".

The sentinel string is now the exported ci.InvalidResolverReport const, referenced from both the fallback that emits it and the orchestrator that matches on it, so the two copies cannot drift.

3. Make the resolver test before it pushes

The resolver's instructions told it to re-run the failing tests, which says nothing about the tests its own edits break. Added an explicit test-and-lint-before-commit requirement to both halves of the prompt — as a properly numbered task step (incrementing the step counter, so it does not render as a bare paragraph wedged between two numbered steps) and as item 6 of the system prompt's "You are NOT done until". Goldens regenerated.

Dropped: the existing-branch-builds fix

All changes to go/internal/fast/build.go, go/internal/prompts/gitops/git_init.go and go/internal/roles/gitops/workspace.go from the original branch are removed. That fix targeted fast.Build, which is not the deployed path; it also contradicted the unmodified git-init system prompt and would double-open PRs. It will be redone separately against orch/build.go as a follow-up.

Test coverage

New tests in go/internal/orch/resolve_test.go:

  • TestResolverPermissionModeGatedToClaude — drives real BuildConfigs for claude_code / codex / open_code and asserts auto / "" / "".
  • TestResolverPermissionModeRespectsExplicitConfig — an explicit mode is never overridden.
  • TestResolveSendsGatedPermissionMode — end-to-end: the permission_mode kwarg the reasoner actually receives is the gated value.
  • TestClassifyRemoteAdvance — table over unchanged / unknown-before / unknown-after / ours / third-party / unknown-local-HEAD.
  • TestResolverReportInvalidMatchesCISentinel — both fields, plus a real report is not flagged.
  • TestResolveInvalidReportWithOurPushReportsPushedNotFixedpushed=true, fixed=false, report_invalid=true, commit/file lists populated, overall success=false.
  • TestResolveInvalidReportDegradesWhenFetchFailspushed=true, verification_partial=true, no invented detail.
  • TestResolveInvalidReportDoesNotAttributeThirdPartyPushpushed stays false, remote_advanced=true, no commits described.
  • TestResolveInvalidReportRemoteUnchangedClaimsNothing — nothing claimed.

Gates run locally (CI's go job, replicated verbatim on Go 1.23.0, GOWORK=off, with the pinned SDK sparse clone as a sibling)

gofmt -l . clean · go build ./... OK · go vet ./... OK · go test -race -count=1 ./... all 27 packages ok.

No Python files are touched, so the test job is unaffected.

🤖 Generated with Claude Code

@CLAassistant

CLAassistant commented Jul 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@AbirAbbas
AbirAbbas force-pushed the fix/agent-reliability branch from e040e53 to d88c69a Compare July 24, 2026 19:58
AbirAbbas and others added 3 commits August 10, 2026 09:21
…untime

run_pr_resolver was handed cfg.PermissionMode verbatim, which defaults to
"". Under the claude harness an empty permission mode omits
--permission-mode, so `claude --print` falls back to its interactive
"prompting" default; with nobody to answer the prompt every write is
denied and the resolver finishes having produced no commits.

Default to "auto" (→ bypassPermissions) so the resolver can write to the
throwaway clone it owns — but only when the resolved provider is claude.
The other two harnesses must not get "auto":

- codex already yields `--sandbox workspace-write` for an empty mode, so
  the workspace is writable; "auto" would escalate to
  --dangerously-bypass-approvals-and-sandbox and drop the sandbox around
  the whole machine for no benefit.
- opencode never reads PermissionMode, so a value there is inert and only
  implies a guarantee we do not make.

An explicitly configured permission mode is still passed through unchanged
for every provider.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sable

When the harness returns nothing parseable, run_pr_resolver hands back a
deterministic all-false PRResolveResult. The orchestrator took that at face
value, so a run where the agent committed and pushed but failed to emit a
final structured answer was reported as "nothing happened" — and the CI
gate was skipped because pushed was false.

Compare the remote head branch before and after the agent runs (via
ls-remote, since the agent pushes from its own process and leaves this
workspace's tracking refs stale) and record only what the comparison can
actually prove:

- report_invalid is set whenever the sentinel report is seen, so callers
  can never mistake a reconstructed result for a first-hand one.
- The remote tip moved and equals this workspace's HEAD → pushed=true.
  fixed stays false: a landed commit is not evidence that CI passes or
  that review comments were addressed, so overall success (fixed &&
  pushed) correctly stays false.
- The remote tip moved to some other commit → remote_advanced is recorded
  and nothing is attributed. A third party's push is not ours to claim,
  and their commits are not ours to describe.
- The remote did not move → nothing is claimed at all.

Commit and file lists are reconstructed only after `git fetch origin
<branch>` makes the post-push objects resolvable locally. If that fetch
fails the lists stay empty and verification_partial marks them as unknown
rather than as "nothing changed".

The sentinel string itself is now the exported ci.InvalidResolverReport
const, referenced from both the fallback that emits it and the
orchestrator that matches on it, so the two copies cannot drift.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The resolver's instructions told it to re-run the *failing* tests, which
says nothing about the tests its own edits break. Add an explicit
test-and-lint-before-commit requirement in both halves of the prompt:

- the task prompt gets it as a real numbered step (incrementing the step
  counter, so it does not render as a bare paragraph wedged between two
  numbered steps);
- the system prompt gets it as item 6 of "You are NOT done until".

Goldens under testdata/ regenerated to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas force-pushed the fix/agent-reliability branch from d88c69a to 434e4a0 Compare August 10, 2026 13:27
@AbirAbbas AbirAbbas changed the title fix(go): resolver writability, truthful push reporting, existing-branch builds, test-before-push fix(go): gate resolver permission-mode to claude, truthful push reporting, test-before-push prompt Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants