Skip to content

Unify Sepo progress and durable result identity - #507

Open
lolipopshock wants to merge 3 commits into
mainfrom
fix/answer-final-sepo-identity
Open

Unify Sepo progress and durable result identity#507
lolipopshock wants to merge 3 commits into
mainfrom
fix/answer-final-sepo-identity

Conversation

@lolipopshock

@lolipopshock lolipopshock commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Treat live progress as temporary for /answer, /implement, and /fix-pr, using the non-recursive job-scoped GITHUB_TOKEN.
  • Preserve the existing AGENT_PROGRESS_POLICY modes: disabled means no progress, report-only means non-cancellable progress, and enabled means cancellable progress.
  • For all three routes, post the durable result with resolved GitHub auth first, then verify and delete any temporary progress comment with the workflow token.
  • If durable posting fails, preserve response availability by finalizing the temporary progress comment without retaining progress activity; if both paths fail, fail the finalizer.
  • Mark deterministic final results and reject those markers both in the entrypoint envelope and extract-context, preventing PAT-authored results from becoming implicit follow-ups or authorizing command suggestions.
  • Normalize run IDs consistently for progress creation, final-result markers, and cleanup comparison.
  • Leave future none/brief/full final progress-overview presentation as separate policy work.

Verification

  • npm --prefix .agent run test:ci
  • Focused coverage for resolved-auth posting, workflow-token deletion, progress-disabled direct posting, fallback and double-failure paths, current-run cleanup verification, PAT-like sender filtering, command-bearing self-authored comments, and normalized markers

@lolipopshock

Copy link
Copy Markdown
Contributor Author

@sepo-agent /review

@sepo-agent-app sepo-agent-app Bot added the agent Handled by the agent label Aug 21, 2026
@sepo-agent-app

This comment has been minimized.

Comment thread .github/workflows/agent-router.yml
@sepo-agent-app

This comment has been minimized.

@lolipopshock

Copy link
Copy Markdown
Contributor Author

@sepo-agent /answer but the review doesn't make sense? If we just repost the final answer once it's fine? And we can also skip it by filtering out actions?

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown

Yes—the review is technically valid, but its proposed auth-mode restriction is only one solution.

The original problem was dozens of events from repeated progress edits. A single final repost is fine for App/OIDC: it creates at most one skipped workflow run because the sender is a Bot. The edge case is PAT fallback: PAT comments appear User-authored, so the current entrypoint filters may treat that final answer as a follow-up—or as a real command if it contains a live command suggestion.

Filtering is a good fix, but filtering only by github.event.action is insufficient because both real comments and reposted answers are created. A deterministic hidden “Sepo final answer” marker, rejected in both the entrypoint and extract-context, would safely preserve one-time reposting for every auth mode. It would still create one skipped Actions record, but allocate no runner or agent; eliminating even that record requires posting with GITHUB_TOKEN.

If you want that approach applied, send:

@sepo-agent /fix-pr keep one-time final-answer reposting, add a deterministic self-authored final-answer marker filter in the entrypoint and extract-context, and cover PAT plus App/OIDC behavior


codex | gpt-5.6-sol[max] | Macmini-runner-1

@lolipopshock lolipopshock changed the title Restore Sepo identity for final answers Unify Sepo progress and durable result identity Aug 22, 2026
@lolipopshock

Copy link
Copy Markdown
Contributor Author

@sepo-agent /review

@sepo-agent-app

Copy link
Copy Markdown

Rubrics Review

Total Score Verdict Rubrics Scored
92 PARTIAL 9
Dimension Rubric Result Score Evidence
coding_workflow / generic Validate delegated route authorization partial 5/8 Final markers are rejected both before runner allocation and during context extraction. However, the shared implementation workflow selects repost for every route while post-comment marks only implement and fix-pr.
coding_workflow / generic Reuse existing code pass 8/8 Repost, fallback, verification, and deletion are centralized in the existing progress finalization module, with marker handling isolated in a small shared helper.
coding_workflow / generic Keep docs in sync pass 7/7 The lifecycle documentation and setup guide cover token identity, ordering, cleanup, fallback, and policy behavior.
coding_workflow / generic Read docs and linked context first pass 6/6 The design and docs follow GitHub’s documented behavior that ordinary events emitted with GITHUB_TOKEN do not create another workflow run, while App and PAT events can. GitHub documentation
coding_workflow / generic Leave release notes to release preparation pass 8/8 The PR file set does not modify .agent/CHANGELOG.md or other release notes.
coding_workflow / generic Make surgical changes partial 6/8 Most changes are focused implementation, tests, and documentation. The unconditional repost setting, however, also changes add-rubrics and arbitrary reusable implementation_route values beyond the three documented routes.
coding_workflow / generic Make user-facing automation idempotent pass 7/7 Marker insertion replaces existing markers, cleanup verifies the current run before deletion, and failed durable posting reuses the temporary comment.
coding_workflow / generic Surface stateful failures pass 7/7 Durable-post failures are logged and fall back to the temporary comment; failure of both paths is terminal. Focused coverage verifies the double-failure path and cross-run deletion protection.
communication / generic Keep status comments compact pass 6/6 Finalization consistently uses includeActivity: false, and tests confirm the durable result excludes progress activity while retaining the substantive response. Focused test

Notes

  • At current head dfda111, the hosted check passes, and an independent npm --prefix .agent run test:ci also passed.
  • The default /answer, /implement, and /fix-pr paths address the prior PAT re-entry and run-ID findings with focused regression coverage.
  • Smallest follow-up: make post-comment append the final marker whenever its mode is repost, matching post-response, and add one add-rubrics or custom-route regression test.

Findings

  • WARNING: The authorization guard is incomplete for reusable implementation routes. agent-implement.yml enables repost unconditionally, but post-comment.ts marks only implement and fix-pr. If progress is enabled for add-rubrics or another custom implementation route and resolved auth is a PAT, the resulting User-authored durable comment lacks the marker and can re-enter mention or implicit-follow-up routing. This is outside the default progress policy but remains a supported configuration path.

Final Rubric Verdict

PARTIAL


codex | gpt-5.6-sol[max] | Macmini-runner-6

RESPONSE_FILE: ${{ steps.agent.outputs.response_file }}
AGENT_PROGRESS_COMMENT_ID: ${{ steps.agent.outputs.progress_comment_id }}
AGENT_PROGRESS_FINAL_COMMENT_MODE: merge
AGENT_PROGRESS_FINAL_COMMENT_MODE: repost

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent-implement.yml also serves create-action and add-rubrics, but this unconditional repost combines with post-comment.ts marking only implement and fix-pr. If a supported progress-policy override enables progress for a shared route, PAT auth produces an unmarked User-authored final comment; on an agent-labeled target, or when the summary contains a live command, it can re-enter routing. Please either mark every repost-mode result or keep non-implement shared routes on merge, and add focused shared-route coverage.

@sepo-agent-app

Copy link
Copy Markdown

AI Review Synthesis

Dual-agent review by Claude and Codex.
Requested by @lolipopshock.

Summary of PR/Issue

PR #507 makes live progress temporary for /answer, /implement, and /fix-pr. Durable results are posted with resolved GitHub authentication before the verified workflow-authored progress comment is deleted, with merge fallback preserving availability. It also adds deterministic final-response markers to prevent PAT-authored results from re-entering routing and normalizes run IDs consistently. Documentation and focused tests cover the intended three routes.

Review

The primary design is sound, and the prior blocking PAT issue is fixed. One bounded gap remains for other routes sharing the implementation workflow, while the informational findings can remain follow-ups.

Issue Severity Disposition Description
Shared implementation routes can repost unmarked PAT results WARNING FIX_IN_PR The shared workflow enables repost for every implementation route, but post-comment.ts marks only implement and fix-pr; supported progress overrides can therefore produce unmarked PAT-authored results for create-action, add-rubrics, or custom routes.
Marker substring filtering can silently skip user comments INFO FOLLOW_UP Both routing guards recognize the sentinel anywhere in the body, so a user request containing copied raw marked content is discarded without feedback.
Other resolved-auth routes remain outside the marker contract INFO FOLLOW_UP Some routes that already post with resolved authentication remain unmarked; this behavior predates the PR and is outside its three-route scope.

Progress

  • The prior /answer PAT re-entry concern is addressed by entrypoint and extract-context marker guards; its existing inline thread was already resolved.
  • The cleanup run-ID normalization gap is fixed.
  • Review artifacts report the hosted check and independent npm --prefix .agent run test:ci passing at head dfda111.
  • Removal of collapsed progress activity from final statuses is intentional; richer final-progress presentation remains separate policy work.
  • Posted one non-duplicate inline comment for the shared-route gap. No replies, minimizations, or thread resolutions were performed.

Issue Details

Shared implementation routes can repost unmarked PAT results

Cause: The shared workflow unconditionally selects repost, while post-comment.ts limits marker insertion to implement and fix-pr. With progress enabled for another shared route, PAT auth can create an unmarked User-authored event that re-enters routing.

Candidate solutions:

  • Prefer appending the marker whenever finalization mode is repost, matching post-response.ts, and add add-rubrics or custom-route regression coverage.
  • Alternatively, retain merge for shared routes other than implement.

Comments: Codex and the rubric reviewer identified this gap; Claude judged the PR mergeable after validating only the intended default routes. The current source supports a warning rather than a blocker because the regression requires a supported but non-default progress configuration.

Marker substring filtering can silently skip user comments

Cause: The entrypoint contains expression and hasFinalResponseMarker accept the sentinel anywhere in a comment, including copied raw response content.

Candidate solutions:

  • Evaluate a stricter canonical-position check without weakening the PAT guard.
  • Otherwise document the marker as reserved input.

Comments: Claude raised this independently. The behavior is fail-closed and has no authorization or data-loss impact, so it should not delay this PR.

Other resolved-auth routes remain outside the marker contract

Cause: The new marker contract targets the three routes changed here; other existing resolved-auth comment paths were not audited or modified.

Candidate solutions:

  • Follow up with a route-by-route audit and mark only outputs whose resulting events can re-enter the agent router.

Comments: This is pre-existing behavior and should not expand the current fix.

Recommended Next Step

FIX_PR: Apply one bounded marker-condition change for every repost-mode shared implementation result and add focused shared-route regression coverage.

Final Verdict

MINOR_ISSUES

Action Items

  • FIX_IN_PR: Mark every repost-mode result from the shared implementation workflow and add PAT/shared-route regression coverage.

codex | gpt-5.6-sol[max] | Macmini-runner-1

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

Labels

agent Handled by the agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant