Skip to content

Reject carried-forward receipts by default - #47

Open
jamilahmadzai wants to merge 3 commits into
permission-protocol:mainfrom
jamilahmadzai:fix/reject-carried-forward-receipts
Open

jamilahmadzai wants to merge 3 commits into
permission-protocol:mainfrom
jamilahmadzai:fix/reject-carried-forward-receipts

Conversation

@jamilahmadzai

@jamilahmadzai jamilahmadzai commented May 18, 2026 •

Copy link
Copy Markdown

Summary

  • reject carried-forward receipt responses by default
  • add an explicit allow-carry-forward opt-in for repos that knowingly want PR-level approval carry-forward
  • document the stricter default in the README and install guide

Why

A carried-forward approval can keep the gate green after the PR head changes. Since the action cannot prove which head SHA the prior approval covered, the safer default is to fail closed and require a fresh approval for the current head unless carry-forward is explicitly enabled.

Related: #46
Bounty: #36

Validation

  • Merged current upstream main (788dd7b) and retained its PR-title JSON encoding and UTF-16 limit behavior.
  • bash test/run-tests.sh: 90 passed, 0 failed, including upstream title handling and carry-forward policy in both PR and deploy modes.
  • YAML parsing, input-default assertions and shell syntax checks pass.
  • git diff --check passes.
  • The harness stubs GitHub and Permission Protocol requests; it does not provide human authorization or a real-service integration test.

Payout

PayPal: jamilurrehman722@gmail.com

@AAH20 AAH20 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.

Clean and well-scoped. The core change — inserting a CARRIED_FORWARD check between the API response parse and the set_output calls — is exactly where it belongs.

Key observations:

  1. Placement after VALID and before outputs — the guard at line ~375 flips VALID=false and sets PP_CARRY_FORWARD_DISABLED only when all three conditions hold (VALID=true, CARRIED_FORWARD=true, opt-in not enabled). This means a genuinely fresh receipt is never touched, and a failed/denied receipt is never double-errored.

  2. PP_CARRY_FORWARD_DISABLED error-code block — follows the same pattern as SIGNATURE_INVALID/KEY_REVOKED: prints details, calls upsert_pr_comment, exits 1. Consistent with the rest of the action.

  3. README ## Carry-forward approvals section — good call documenting the version boundary (v2.2.0) so users upgrading know when the behavior changed.

  4. One nit (non-blocking): the CARRIED_FORWARD variable is parsed from the API response with a fallback of "false". If the API ever returns the boolean false rather than the string "false", jq -r renders it as false which matches, so this is fine — just noting for posterity.

LGTM. Approving.

@permission-protocol

permission-protocol Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Permission Protocol: human approval required

No automatic clearance rule applies.

Review & sign · Reject

What changes: This pull request changes product or deploy behavior in the areas described by the engineering team. Reviewers may see a different result in the affected workflow after this is released. Operators may need to watch the affected flow closely on the first deploy.

Estimated risk: high. Sensitive executable, configuration or receipt-gate changes need review; production impact must be confirmed from the diff.
Read 4/4 changed file records; 0 patches incomplete or unavailable.

  • action.yml (modified, source or configuration): Executable or configuration content changed in a sensitive review area; inspect the diff to establish its runtime consequence. Review: Verify the changed behavior preserves access, data and authorization requirements.
  • test/run-tests.sh (modified, test or sample): This test or sample changes; its path alone does not establish production impact. Review: Check references and imports before assuming this file is isolated from production.
  • INSTALL.md (modified, documentation): Readers may follow incorrect instructions if this documentation is inaccurate. Review: Check the instructions and references against the supported behavior.

Inspect the supporting diff and all 4 changed files

Path-based review areas: Infra, Scripts. These matches do not establish production behavior changes.

Note: Structured review generation failed — deterministic fallback used for review bullets.
Note: Approval brief was not generated from this diff — it is inferred from which files changed. Confirm the intent and the verification steps against the diff before signing.

Signing authorizes this commit and scope. It does not prove deployment; other GitHub and deployment requirements still apply.

See eligible approvers in Permission Protocol

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