Skip to content

fix(approvals): tell an expired approval apart from a rejection - #674

Open
Eunyoung Jeong (eunyoung14) wants to merge 3 commits into
mainfrom
fix/502-expired-approval
Open

Eunyoung Jeong (eunyoung14) wants to merge 3 commits into
mainfrom
fix/502-expired-approval

Conversation

@eunyoung14

Copy link
Copy Markdown
Member

An agent running commands through alpacon exec couldn't tell a timed-out approval from a person saying no. A lapsed purpose demand also didn't say where the command went.

Changes

  • Expired approvals: rejection_cause=approval_expired now reads "was not approved before the approval request expired" on every command path: exec, exec --wait (the poll loop now keeps the cause and treats status=rejected as terminal) and exec logs. A human rejection keeps "was rejected by a reviewer". An older server that sends no cause keeps today's wording.
  • JSON output: the --output json envelope carries a stable error_code per cause: approval_expired, approval_rejected, and the other causes the server sends. It's the same on all three paths. Older servers keep not_approved.
  • Exit codes: unchanged.
  • Purpose demand: the message gives the absolute deadline in local time next to the seconds left. It says what happens after the deadline: the command is judged again without a purpose, then runs, waits for a human approver (who is notified then), or is refused. It points to alpacon exec logs <id>.
  • exec logs <id> on a command that went through a purpose demand reports where it landed: running or finished, awaiting approval (with the approval request id), or refused (with the cause). The request id lookup stays quiet on 403, 404 or an older server, and stops paging at the first match. Output for an ordinary command is unchanged.
  • Help text for exec and work-session create: the session purpose is what each command is judged against. Pass --purpose with each command to skip the demand.

The 60-second purpose window and the server state model are unchanged.

Tests

  • Cause mappings and messages on all three paths, including wait-path tests that enter the poll loop.
  • Cross-path error_code, and older-server fallbacks.
  • The purpose-demand deadline and lapse outcomes.
  • Approvals lookup fallbacks and pagination.
  • The exec logs notice, shown only after a purpose demand.

The regression tests fail without the change. go build, go vet, go test -race ./... and golangci-lint (0 issues) all pass.

Co-Authored-By: Codex <noreply@openai.com>
…provals lookup

Co-Authored-By: Codex <noreply@openai.com>
Copilot AI lite review requested due to automatic review settings October 30, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The deadline message is ambiguous because it omits the local date and timezone, making demands that cross midnight or are read later difficult to interpret.

1 open issue
What changed in this PR

This PR improves approval and purpose-demand diagnostics for alpacon exec, distinguishing expired approvals from reviewer rejections and reporting where commands land after a purpose demand expires.

Changes:

  • Propagates rejection causes and stable JSON error_code values across execution, approval waits, and exec logs.
  • Adds approval-request lookup and purpose-demand outcome reporting to command logs.
  • Expands deadline guidance, help text, and regression coverage for approval and purpose-demand flows.
File Summary Review notes
utils/​purpose_demand.go Adds deadline and post-expiry guidance to purpose-demand messages. Moderate: Include the local date and timezone in the deadline, not only the time, so deadlines crossing midnight remain unambiguous.
utils/​purpose_demand_test.go Tests deadline and lapse guidance in table and JSON output. —
cmd/​worksession/​worksession_create.go Documents session-purpose behavior. —
cmd/​exec/​use_file_execution_test.go Covers cause-specific messages and JSON codes across execution paths. —
cmd/​exec/​run.go Preserves rejection causes through the approval wait loop. Moderate: Preserve the legacy expired outcome when sudo_grant_status=expired is returned without a rejection cause, while treating cause-bearing rejected responses as terminal.
cmd/​exec/​run_test.go Updates approval wait message expectations. —
cmd/​exec/​purpose_demand_test.go Tests purpose-demand lapse outcomes and approval lookup output. —
cmd/​exec/​pending_approval_test.go Tests approval lookup fallback behavior. —
cmd/​exec/​logs.go Reports purpose-demand outcomes and approval request IDs. —
cmd/​exec/​logs_test.go Tests completion reporting after purpose demands. —
cmd/​exec/​exec.go Updates exec help for purpose-demand expiry behavior. Nit: Update the related purpose help and README exit-code documentation, which still describe the old post-expiry behavior.
api/​event/​types.go Adds approval cause constants, messages, and error-code mapping. Moderate: Emit approval_expired only when a cause is present so legacy expiry responses retain the not_approved JSON fallback.
api/​event/​event_test.go Tests rejection-cause messages and error-code mapping. —
api/​approval/​types.go Adds command ID decoding to approval metadata. Moderate: Decode and match the nested metadata.command.command_id shape used by command approvals, while retaining a fallback for other server shapes.
api/​approval/​approval.go Adds paginated lookup of pending command approval requests. —
api/​approval/​approval_test.go Tests approval lookup pagination and early termination. —

Review effort: Lite

Comment thread utils/purpose_demand.go Resolved
Match nested command approval metadata with the existing flat fallback. Preserve a cause-less expired grant when the command is also rejected.

Co-Authored-By: Codex <noreply@openai.com>
@eunyoung14

Copy link
Copy Markdown
Member Author

Review-body findings addressed in d5e8e70:

Preserve the legacy expired outcome when sudo_grant_status=expired is returned without a rejection cause, while treating cause-bearing rejected responses as terminal.

Fixed at cmd/exec/run.go:511. A cause-less expired grant keeps its expired outcome even when the command status is rejected. An explicit reviewer rejection remains authoritative. TestPollApprovalOnceLegacyExpiry covers both legacy status shapes and the explicit rejection case.

Decode and match the nested metadata.command.command_id shape used by command approvals, while retaining a fallback for other server shapes.

Fixed at api/approval/approval.go:123 and api/approval/types.go:35. The lookup reads the nested command ID, keeps the flat fallback, and stops at the first match. TestFindCommandApprovalRequestIDEarlyStop covers both shapes and an empty nested object.

Emit approval_expired only when a cause is present so legacy expiry responses retain the not_approved JSON fallback.

api/event/types.go:335 already maps the legacy expired outcome to approval_expired. The error-code values and exit codes are frozen for this PR, so that mapping is retained. A response with neither a cause nor an expired grant still uses the existing fallback.

Update the related purpose help and README exit-code documentation, which still describe the old post-expiry behavior.

cmd/exec/purpose.go:26 and README.md:571 contain the referenced prose. The approved scope freezes wording, so those suggested copy changes are deferred. The deadline-format finding is answered on its inline thread for the same reason.

Both regressions failed before the fixes. The build, vet, full race-test suite, and lint all pass.

Applied the two logic fixes with regression tests and retained frozen wording and code mappings; the fix commit can be reverted independently.

@alpacax-wall-d alpacax-wall-d Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WALL-D review: Approval withheld · head d5e8e70

Code review result: Comments only (1 P1/P2) · head d5e8e70

Review summary

  • Found 1 P1/P2 defect.

A maintainer will review this PR.

Related PRs

Product review result: Check needed (2 items) · head d5e8e70
A maintainer will follow up on the product items.

Re-review requests and limits

WALL-D reviews a PR opened ready after this rule took effect once on its own. After that it reviews only on request: comment @alpacax-wall-d review or mark a draft ready for review, up to 5 times per PR including the first review. A re-review reads only the lines changed since the last review, and from the second re-review on only P1 findings block approval. Answering the product review's questions reviews only the product side again.

Comment thread api/event/types.go
Comment on lines +339 to +341
if e.Expired {
return RejectionCauseApprovalExpired
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 · contract Legacy expiries without a rejection cause no longer preserve the fallback error code.

Failure scenario: An approval poll receives sudo_grant_status=expired without rejection_cause → ErrorCode returns approval_expired through Expired, while the equivalent cause-free exec and exec logs paths retain not_approved.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Intended, so no code change: on the exec --wait sudo-grant poll the client learns the expiry itself from sudo_grant_status=expired, and api/event/types.go:339 reports approval_expired for it. The cause-free exec and exec logs paths keep not_approved because there the client can't tell an expiry from a reviewer's rejection. In both cases the code reflects what the client can observe, and an expiry it knows about is never reported as a rejection. The full answer is in the PR comment.

@eunyoung14

Copy link
Copy Markdown
Member Author

Answers to the WALL-D review on d5e8e70:

P2, api/event/types.go:341 (legacy expiry error code): this is intended, and the code stays as it is. On the exec --wait sudo-grant poll, the client learns the expiry from sudo_grant_status=expired itself, so Expired is true even when the server sends no rejection_cause. Reporting approval_expired there states what the client actually knows. The cause-free exec and exec logs paths keep not_approved because there the client can't tell an expiry from a reviewer's rejection. In both cases the code matches what the client can observe; an expiry the client knows about is never reported as a rejection.

Product item, release notes: this ships as a fix in the next CLI release notes. Expired approvals no longer read as a reviewer's rejection, --output json carries a per-cause error_code, and a purpose demand states its deadline and what happens after it. Exit codes are unchanged, so scripts that branch on them need no change. Scripts that want to tell causes apart can read error_code.

Product item, documentation follow-up: planned, not deferred. The public CLI reference for alpacon exec (--output json error codes and the purpose-demand message) is updated in the documentation release that ships with this CLI release. The in-repo help text for exec and work-session create already changed in this PR (cmd/exec/exec.go:37, cmd/worksession/worksession_create.go:57).

@alpacax-wall-d review

@alpacax-wall-d alpacax-wall-d Bot added the needs-author-fix WALL-D: fix needed on the head (P1/P2 finding, product fix-before-merge item, or failing CI) label Oct 30, 2026
@alpacax-wall-d

Copy link
Copy Markdown

No changes in d5e8e70..d5e8e70.
#674 (comment)

@alpacax-wall-d alpacax-wall-d Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WALL-D review: Approval withheld · head d5e8e70 · Product review result: Nothing flagged

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

Labels

needs-author-fix WALL-D: fix needed on the head (P1/P2 finding, product fix-before-merge item, or failing CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants