fix(custody): close bulk_reject self-verify gap + harden verify/review guards (parity with #252)#253
Merged
Merged
Conversation
The three public custody guard functions (verify_allowed?, review_conducted?,
mark_complete_allowed?) return :ok | {:error, _}, not booleans. Using ? naming
violates Elixir convention and creates a dangerous footgun: misuse as a
predicate (if Progress.verify_allowed?(...), do: ...) silently disables the
guard since the error tuple is truthy.
Rename to non-predicate names:
- verify_allowed? → ensure_verify_allowed
- review_conducted? → ensure_review_conducted
- mark_complete_allowed? → ensure_mark_complete_allowed
This signals to callers that these are validation functions returning :ok | {:error, _}
and must be used with 'with' or pattern matching, not as predicates.
Also:
- Add documentation that nil orchestrator_agent_id is treated as untrusted
- Update bulk_mark_complete MCP tool description to clarify backfill-only restriction
Fixes the three Medium/Low findings from enhanced review of PR #252.
…fy/mark-complete and bulk reject Address all five findings from PR-1 enhanced review: 1. Guard renames: verify_allowed? → ensure_verify_allowed, etc. (Already completed in prior work) 2. Add self-verify guard to bulk_reject: enforce chain-of-custody parity by blocking self-adjudication in bulk reject path (same guard as verify_story/4 single-path) 3. Validate completed_at in review records: add two checks: - completed_at must not be >5 minutes in the future - completed_at must be after story.reported_done_at Both validations happen in ReviewRecord.create_changeset + Progress.record_review 4. Block re-verifying already-verified stories in bulk: check verified_status==:verified in validate_verify_preconditions and reject with :already_verified error (already formatted by format_reason/1) 5. Update @doc for ensure_verify_allowed: document nil identity behavior as 'always blocked' per chain-of-custody model Add 2 new tests for findings 2 and 4: - test 'bulk reject blocks self-adjudication' - test 'bulk verify blocks re-verification of already-verified story' All 3096 tests pass; no findings deferred per NO-DEFERRALS rule.
Follow-up refinements to the bulk custody-parity branch (on top of the
rename + guard commits):
- Revert an unrequested Progress.record_review guard that rejected
completed_at < reported_done_at with {:error,
:review_completed_before_reported_done}. The review-record controller's
case had no clause for that atom, so a backdated review crashed the
endpoint (500). Stale reviews are already blocked at verify time by
ensure_review_conducted/3, so record_review needs no extra guard.
- FIX 5: add ReviewRecord.create_changeset unit tests
(test/loopctl/artifacts/review_record_test.exs) covering far-future
(invalid), just-past-tolerance (invalid), near-now and now (valid);
correct the @doc to describe only the 5-minute future check.
- FIX 4: strengthen the already-verified bulk-verify test into a real
verify-then-re-verify flow, asserting verified_at is unchanged and
exactly one VerificationResult exists after the blocked re-verify.
- FIX 6: update the stale mcp-server README bulk_mark_complete row to
backfill-only (never-dispatched) semantics.
Refs private advisory GHSA-h49f-rhf8-8p6f.
Companion to the review-record test commit — carries the code and doc
changes that belong with it:
- progress.ex: revert the unrequested record_review completed_at <
reported_done_at guard. It returned {:error,
:review_completed_before_reported_done}, an atom the review-record
controller's case does not match, so a backdated review crashed the
endpoint (500). Stale reviews are already blocked at verify time by
ensure_review_conducted/3.
- review_record.ex: correct the create_changeset @doc to describe only
the 5-minute future check (the changeset never saw reported_done_at).
- bulk_operations_controller_test.exs: strengthen the already-verified
bulk-verify test into a real verify-then-re-verify flow, asserting
verified_at is unchanged and exactly one VerificationResult exists.
- mcp-server/README.md: update the stale bulk_mark_complete row to
backfill-only (never-dispatched) semantics.
Refs private advisory GHSA-h49f-rhf8-8p6f.
mkreyman
changed the title
Enhanced-review (Sonnet-5/xhigh) flagged that the 5-minute future tolerance on review-record completed_at is generous for clock skew and leaves a bounded window where a forward-dated review can cover a later, unreviewed re-report. Reduce the allowance to a 60s clock-skew constant and document that the full structural closure (binding a review record to the report generation it covers) is tracked with the chain-of-custody invariant work. Refs private advisory GHSA-h49f-rhf8-8p6f.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix-forward for the lateral chain-of-custody gaps an enhanced review found in PR #252 (bulk verify / mark-complete custody guards). All findings are addressed in this one branch — no deferrals.
Fixes
1. [Medium — footgun] Rename
?-suffixed guards that don't return booleans.verify_allowed?/2,review_conducted?/3,mark_complete_allowed?/1returned:ok | {:error, atom}, so the?names violated the predicate convention (anif guard?(...)would treat the error tuple as truthy and silently disable the check). Renamed toensure_verify_allowed/2,ensure_review_conducted/3,ensure_mark_complete_allowed/1;@doc/@spec/all call sites updated. Contract (:ok | {:error, _}) unchanged.2. [Low — doc] Document nil-identity blocking.
ensure_verify_allowed/2@docnow states a nilorchestrator_agent_idis treated as an untrusted identity and is always blocked (maps to{:error, :self_verify_blocked}).3. [High — security]
bulk_rejectwas missing the self-verify guard.Single-story
Progress.reject_storyrunsvalidate_not_self_verify, butBulkOperations.process_rejectdid not — so an orchestrator could reject (self-adjudicate) their own implemented work, and rejection is destructive (auto_reset_agent_statuswipesassigned_agent_id/reported_done_at). AddedProgress.ensure_verify_allowed(story, orchestrator_agent_id)toprocess_reject'swithchain, mirroringprocess_verify. A self-reject now returns anerrorresult with reason=~ "chain-of-custody"and the story stays:unverified.4. [Low — security]
bulk_verifyallowed re-verifying an already-verified story.validate_verify_preconditionsonly checkedagent_status == :reported_done; the single path (Progress.validate_verifiable) also blocksverified_status == :verified. Added an orderedcondso already-verified is rejected first with:already_verified.5. [Medium — security] Review-record
completed_atwas unvalidated.ReviewRecord.create_changesetcast client-suppliedcompleted_atwith no bound, so a future-dated review permanently satisfiedensure_review_conducted(which only checkscompleted_at > reported_done_at). Added avalidate_changerejecting timestamps more than 5 minutes in the future.6. [Low — docs] Stale MCP
bulk_mark_completedescription.Updated the tool description (
mcp-server/index.js) and the README summary row to state it is backfill-only for pre-existing, never-dispatched stories; dispatched stories must use the normal verify flow. Doc-string only.Review-hardening note (this branch's follow-up commits)
An interim commit added an extra
Progress.record_reviewguard rejectingcompleted_at < reported_done_atwith a new:review_completed_before_reported_doneatom. The review-record controller'scasehas no clause for that atom, so a backdated review would have crashed the endpoint (500). It was also out of scope — stale reviews are already blocked at verify time byensure_review_conducted/3. Reverted to keep the endpoint safe and the PR scoped to the six findings.Tests
test/loopctl/artifacts/review_record_test.exs(new) — FIX 5: far-future and just-past-5-mincompleted_atinvalid; now / near-now valid.bulk_operations_controller_test.exs— FIX 3:POST /stories/bulk/rejectself-adjudication blocked (422, reason=~ "chain-of-custody", story stays:unverified). FIX 4: real verify-then-re-verify flow assertingverified_atunchanged and exactly oneVerificationResultafter the blocked re-verify.Verification
mix compile --warnings-as-errors,mix format --check-formatted,mix credo --strict,mix dialyzer(0 errors), and fullmix test(3100 tests, 0 failures, 40 excluded) all green. Committed through the full pre-commit gate (no--no-verify).Refs private advisory GHSA-h49f-rhf8-8p6f.