Skip to content

fix(#7000): resolve role from harness config for status reconciliation - #7167

Merged
rh-hemartin merged 2 commits into
mainfrom
agent/7000-reconcile-role-from-harness
Sep 10, 2026
Merged

fix(#7000): resolve role from harness config for status reconciliation#7167
rh-hemartin merged 2 commits into
mainfrom
agent/7000-reconcile-role-from-harness

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

  • Fix mint 403 errors during status reconciliation for custom agents (e.g., grillme) that declare a role different from their agent name in the harness config
  • Emit the harness-resolved role as a GITHUB_OUTPUT from fullsend run so the finalize step in action.yml can pass it to reconcile-status --role
  • Fall back to the agent name when the output is absent, preserving backward compatibility with older CLI versions

Changes

  • internal/cli/run.go: After the pre-script relay, write h.Role to GITHUB_OUTPUT using the existing writeGitHubOutput helper, gated on GITHUB_ACTIONS=true
  • action.yml: Read steps.run.outputs.role as HARNESS_ROLE env var in the finalize step; use ${HARNESS_ROLE:-${AGENT}} for the --role flag
  • internal/cli/reconcilestatus_test.go: Add test verifying a custom role (review) passes through to the MintRequest correctly

Test plan


Closes #7000

Post-script verification

  • Branch is not main/master (agent/7000-reconcile-role-from-harness)
  • Secret scan passed (gitleaks — 34f2d076c5f6c3249c2e25467341ed5495518f13..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

The finalize step in action.yml passed the raw agent name (e.g.,
"grillme") as --role to reconcile-status. Custom agents that
declare a different role in their harness config (e.g., role:
review) caused mint 403 errors because the agent name is not an
enrolled mint role.

Thread the harness-resolved role through GITHUB_OUTPUT from
fullsend run so the finalize step can use it. The action falls
back to the agent name when the output is absent (backward
compatibility with older CLI versions).

Closes #7000
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner September 9, 2026 14:40
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Sep 9, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ❌ Failure (validation failed after 1 iteration(s)) · Started 2:47 PM UTC · Completed 2:49 PM UTC

Commit: d0c3e3f · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:03 PM UTC · Completed 3:22 PM UTC

Commit: d0c3e3f · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.03

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 9, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 9, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Re-review: Tier 1 signals (file count, blast radius, zero protected paths, no dependency/CI changes, bot author) are unchanged from the prior assessment, and the only delta since that review is a same-logic reorder in run.go plus corresponding test-assertion updates - no new risk surface - so the prior moderate score of 2 is preserved, still weighted down from low by run.go and action.yml persistently high-churn, multi-author, high fix/revert-rate history.

Previous run

Risk Assessment: moderate (2/5)

Details

Small, well-scoped bot-authored PR with no protected/security paths or CI/dependency changes that closes out a well-scoped production incident with a backward-compatible fallback, but internal/cli/run.go and action.yml are unusually high-churn, multi-author, high fix/revert-rate files with substantial coupling to files not touched in this PR, which keeps the composite at moderate rather than low.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 9, 2026

Copy link
Copy Markdown

Looks good to me

Previous run

Review

Findings

High

  • [test-regression] internal/cli/run.go:1615 — The new role-emission block appends role=<h.Role> to GITHUB_OUTPUT on every runAgent path that reaches prescript.Relay. role is a required harness field, validated as non-empty at load time (internal/harness/harness.go:470), so this branch fires whenever GITHUB_ACTIONS=true and GITHUB_OUTPUT is set. Three existing tests in internal/cli/prescript_run_test.go build a harness via newSkipHarnessDir, which declares role: test, and assert exact-match content on GITHUB_OUTPUT: TestRunAgent_PreScriptNoSkip_ProceedsToSandboxAndRelaysFalse (line 119, expects "skipped=false\n"), TestRunAgent_NoPreScript_StillRelaysSkippedFalse (line 140, same), and TestRunAgent_PreScriptSkip_RelaysSkippedTrue (line 159, expects "skipped=true\nreason=open PR exists\n"). All three will fail because the file now additionally contains a role=test line — this breaks go test ./internal/cli/... for this PR. Separately, the PR's only new test (TestNewReconcileStatusCmd_MintSuccess_CustomRole, internal/cli/reconcilestatus_test.go:162) only re-exercises the pre-existing reconcile-status --roleMintRequest passthrough (a no-op alias for role: review) and never touches the new run.go code path, so the actual fix for Mint status reconciliation should resolve role from harness config, not agent name #7000 currently has zero test coverage.
    Remediation: Update the three exact-match assertions in internal/cli/prescript_run_test.go (lines 119, 140, 159) to also expect role=test — e.g. parse key=value pairs or use assert.Contains per expected line. This both unbreaks the suite and supplies patch coverage for the new writeGitHubOutput(..., "role", h.Role) call, since the harness role (test) already differs from the agent name (code) in these fixtures.

Low

  • [design-completeness] internal/cli/run.go:1614 — Role emission runs after setupStatusNotifier, provider/preflight validation, and pre-script execution, all of which can fail or return before this line executes. If a run fails at one of those earlier stages, GITHUB_OUTPUT will lack the role key and action.yml's finalize step falls back to ${AGENT} — the original Mint status reconciliation should resolve role from harness config, not agent name #7000 failure mode — for exactly the crash/failure scenarios where orphaned-status reconciliation matters most. This mirrors the pre-existing skipped key's identical failure window, so it is not a regression introduced by this PR, but consolidating both emissions earlier in runAgent (immediately after harness load/validation) would close the gap for both.
    Remediation: Consider moving the role emission (and ideally the skipped-relay) earlier in runAgent, right after the harness is loaded and validated.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 3:24 PM UTC · Completed 3:36 PM UTC

Commit: d0c3e3f · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.25

The role-emission block added a `role=<h.Role>` line to GITHUB_OUTPUT on
every run, which broke three exact-match assertions in
prescript_run_test.go that predate this change. Update those assertions
to expect the new line, which also gives the new run.go code path patch
coverage (the fixtures declare role: test, distinct from the "code" agent
name, so the assertion proves h.Role, not the agent name, was relayed).

Also move the role emission to immediately after the harness loads
(where h.Role is already validated non-empty), instead of after the
pre-script relay. This narrows the failure window called out in review:
a validation step that aborts the run before the pre-script relay would
previously leave GITHUB_OUTPUT without a role key, causing action.yml's
finalize step to fall back to the raw agent name -- the original #7000
failure mode.

Addresses review feedback on #7167
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed the High-severity test regression by updating three exact-match GITHUB_OUTPUT assertions to expect the new role=test line, which also supplies the missing patch coverage for the run.go role-emission path. Addressed the Low-severity design-completeness finding by moving role emission to immediately after harness load/validation, narrowing the window where an early failure would leave GITHUB_OUTPUT without a role key.

Fixed (2):

  1. High: new role-emission block in internal/cli/run.go breaks three exact-match GITHUB_OUTPUT assertions in internal/cli/prescript_run_test.go, and the PR's only new test never exercises the run.go code path, leaving the actual Mint status reconciliation should resolve role from harness config, not agent name #7000 fix with zero coverage (internal/cli/prescript_run_test.go): Updated the three exact-match assertions (TestRunAgent_PreScriptNoSkip_ProceedsToSandboxAndRelaysFalse, TestRunAgent_NoPreScript_StillRelaysSkippedFalse, TestRunAgent_PreScriptSkip_RelaysSkippedTrue) to expect the new role=test line. Since the fixtures declare role: test while runAgent is invoked with agent name "code", these assertions now directly prove h.Role (not the agent name) is what gets relayed, closing the coverage gap the reviewer flagged without adding a redundant new test.
  2. Low: role emission runs late in runAgent, so an early abort leaves GITHUB_OUTPUT without a role key and action.yml falls back to the agent name (internal/cli/run.go): Moved the role-emission block to immediately after the harness loads (where h.Role is already validated non-empty by harness.Load), rather than after the pre-script relay. This narrows the failure window for the role output specifically. Did not move the pre-existing skipped-relay logic the reviewer mentioned as an optional additional step ("ideally"), since that is unrelated pre-existing code extensively covered by other tests and moving it is out of scope for this PR's Mint status reconciliation should resolve role from harness config, not agent name #7000 fix.

Tests: passed

Decision points
  • Scoped the Low-severity design-completeness fix to only the new role-emission code, not the pre-existing skipped-relay mechanism (alternatives: Move both role emission and the skipped-relay earlier in runAgent as the reviewer suggested ('ideally'), Move only the role emission (this PR's own new code) and leave the skipped-relay untouched, Disagree entirely and leave role emission in its original late position; rationale: The reviewer marked this Low severity, explicitly said it mirrors pre-existing behavior and 'is not a regression introduced by this PR', and hedged the skipped-relay move with 'ideally'. Moving only the role emission is a small, self-contained, low-risk change fully traceable to the finding and directly strengthens the PR's own Mint status reconciliation should resolve role from harness config, not agent name #7000 fix. Moving the skipped-relay too would touch unrelated, already-tested pre-existing behavior for no benefit to this PR's scope.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 9, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 3:38 PM UTC · Ended 3:53 PM UTC

Commit: c3ab21c · View workflow run →

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review September 9, 2026 15:53

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 9, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:38 PM UTC · Completed 3:53 PM UTC

Commit: c3ab21c · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.46

@rh-hemartin
rh-hemartin added this pull request to the merge queue Sep 10, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 10, 2026
@rh-hemartin
rh-hemartin added this pull request to the merge queue Sep 10, 2026
Merged via the queue into main with commit e57fcc2 Sep 10, 2026
53 checks passed
@rh-hemartin
rh-hemartin deleted the agent/7000-reconcile-role-from-harness branch September 10, 2026 09:26
@fullsend-ai-retro

fullsend-ai-retro Bot commented Sep 10, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:28 AM UTC · Completed 9:37 AM UTC

Commit: c3ab21c · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.55

@fullsend-ai-retro

Copy link
Copy Markdown

PR #7167 (fullsend-ai/fullsend) fixed mint 403 errors during status reconciliation for custom-role agents (e.g. grillme), closing #7000 and duplicate #7164. Triage (#7000) pinpointed the exact root cause and fix shape (thread h.Role through GITHUB_OUTPUT in run.go; read it in action.yml's finalize step with a fallback to ${AGENT}), and the code agent implemented that plan faithfully. The first review (against commit d0c3e3f) correctly flagged a High-severity test regression: the new unconditional role=<h.Role> output line broke 3 pre-existing exact-match GITHUB_OUTPUT assertions in internal/cli/prescript_run_test.go (same package), which independently caused real CI and E2E failures on that commit per gh pr checks. It also correctly noted the new test never exercised the actual fixed code path, and flagged a Low-severity ordering issue (role emission happening after early-return-prone steps). The fix agent resolved both in one iteration; the re-review returned a clean 'Looks good to me' against c3ab21c. A human (rh-hemartin) approved without additional comments ~15 hours later (overnight gap, not a quality issue) and the PR merged. This is a case where the review/fix loop worked as intended: a real, CI-confirmed regression was caught and corrected within a single fix iteration, and the human's uncommented approval reflects that the agent review had already done the substantive verification.

The one rework driver — the code agent scoping its pre-PR test run too narrowly ('existing reconcile-status tests pass' rather than the whole internal/cli package, which would have caught the same-package breakage) — is already tracked as an open issue, fullsend-ai/agents#492, filed after a near-identical fix-agent incident (package-scoped go test ./internal/config/... missing cross-file breakage). #492 already proposes reworking skills/code-implementation/SKILL.md step 9c and skills/fix-review/SKILL.md to prefer repo-wide/package-level test runs over narrow scoping, and to defer to any stricter repo-documented test policy (this target repo's docs/contributing/go-code.md already mandates unconditional go test ./..., which the generic skill guidance doesn't currently defer to). This PR's incident is corroborating evidence for #492, not a new gap — same root cause, different package. #1719 (code agent should run repo CI commands before pushing) is a related but distinct prior report.

Separately, the underlying bug class here (harness-declared role diverging from agent name, breaking assumptions elsewhere in the pipeline) is already known to be recurring: #7000 and #7164 were filed independently by two different reporters within minutes of each other on 2026-09-09, both converging on the same mint-403 root cause — evidence the bug was actively affecting multiple custom-agent users concurrently. The deeper upstream cause (bash-identifier restrictions forcing hyphen-free role: overrides, which is what creates the name/role divergence in the first place) is already tracked as open issue #7140, and the related opaque-403-for-unregistered-role UX gap is tracked as open issue #6563. No new proposal is warranted for either; this retro's evidence (two independent same-day reporters) supports keeping #7140 prioritized, which I've noted here rather than filing a duplicate.

No new proposals are being filed — the identified gaps are already covered by open issues (#492, #7140, #6563), and the review/fix cycle on this PR itself functioned correctly with no missed defects or false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mint status reconciliation should resolve role from harness config, not agent name

1 participant