Skip to content

allow create-task without an assignee - #14

Merged
bborbe merged 1 commit into
masterfrom
fix/allow-empty-assignee-on-create
Jul 30, 2026
Merged

allow create-task without an assignee#14
bborbe merged 1 commit into
masterfrom
fix/allow-empty-assignee-on-create

Conversation

@bborbe

Copy link
Copy Markdown
Owner

Problem

validateCreateTaskFrontmatter rejected any create-command with an empty assignee. That makes a task which is born parked unrepresentable.

github-pr-watcher's untrusted-author path (pkg/watcher.go:589, buildHumanReviewFrontmatter) deliberately stamps:

"task_type": "pr-review", "assignee": "", "phase": "human_review", "status": "todo"

So every untrusted-author PR produced a command this controller refused:

I0728 20:49:46.780606 cdb_command-object-executor-tx-result-sender.go:52]
handler command 'create-task' failed with error: validate frontmatter:
frontmatter missing required field: assignee: validation error

No task file, no signal on the PR, and the only evidence is that one log line. Observed on bborbe/git-sync#5 (2026-07-28): the watcher logged published CreateTaskCommand … trusted=false at the same second, Kafka offset 54177 was consumed, and nothing was ever written. The PR sat at BLOCKED with green CI until a human happened to look.

Why remove the check rather than change the watcher

An empty assignee is the operator-inbox signal — the "Make Parked Agent Tasks Visible to Operator" doctrine defines escalation as clearing the assignee so no agent claims the task. The controller already honours that everywhere else:

  • pkg/scanner/vault_scanner.go treats an empty assignee as unclaimed and dispatches nothing
  • The same state is accepted on update — only create rejected it

So this aligns create with update and dispatch. The alternative (watcher stamps a sentinel like assignee: unassigned) would fork the doctrine and require every inbox query to special-case a magic string.

status remains required.

Verification

make precommit green: 12 packages, 0 lint issues, 0 vulnerabilities (gosec + trivy + osv-scanner).

The test that asserted the old behaviour is inverted to assert the new one — a command with assignee absent, status: todo, phase: human_review is now written exactly once.

Post-deploy check: an untrusted-author PR should produce a task file with phase: human_review and assignee: "", per gate 11 of the "Agent - Debug Missing PR Reviewer Review" runbook (which previously documented a file that never actually appeared).

An empty assignee is the operator-inbox signal, so requiring it made a
task that is born parked unrepresentable. The pr-watcher's untrusted-author
path was rejected every time, so those PRs never reached the queue.
status stays required.

@ben-s-pull-request-reviewerben-s-pull-request-reviewerBot 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.

Based on my review, here is the consolidated report and verdict:


PR Review: allow create-task without an assignee

Selector Mode Traceability

  • Candidates: ~100+ judgment rules from index (Go files touched)
  • Applicable: 0 — The diff makes a targeted validation-change (removes assignee requirement, inverts one test case, updates changelog). No judgment rules are violated by these specific changes.
  • Skipped: All judgment rules — their applies_when conditions do not match the scope of this narrow bugfix PR (no architectural decisions, no handler changes, no agent/command files, no factory patterns)

Step 3 — Automated Checks

License: LICENSE file present in repo root ✓

Precommit (selector mode): Skipped — selector mode defers to CI for lint+test.


Mechanical Findings (from Step 4a — pre-computed funnel)

The funnel ran over 66 YAMLs and reported 32 findings across task_create_task_executor.go and task_create_task_executor_test.go. All findings are in pre-existing code that this PR did not modify:

FileFindingSeverityIn PR Diff?
task_create_task_executor.go:256go-context/cancel-check-in-loop — loop missing ctx.Done() checkSHOULDNo
task_create_task_executor.go:300go-logging/no-log-and-return-errorMUSTNo
task_create_task_executor.go:305go-context/cancel-check-in-loopSHOULDNo
task_create_task_executor.go:329go-context/cancel-check-in-loop + go-logging/no-tight-loop-without-samplerSHOULDNo
task_create_task_executor.go:244go-logging/no-sensitive-data-in-logs (token in log arg)MUSTNo
task_create_task_executor.go:476go-glog/use-v-for-debug-not-info — bare glog.Infof at V0MUSTNo
task_create_task_executor_test.gogo-testing/no-bare-error-call (22 instances)MUSTNo
task_create_task_executor_test.go:29go-testing/suite-test-file-requiredMUSTNo

None of these violations are introduced by this PR. They are all in the auto-supersede feature (lines 215–511) and test mock setup that existed before this change. The validateCreateTaskFrontmatter function (lines 189–206) had its assignee check removed — no new violations were introduced.


Step 7 — Manual Review

Changed files reviewed: pkg/command/task_create_task_executor.go, pkg/command/task_create_task_executor_test.go, CHANGELOG.md

validateCreateTaskFrontmatter change (task_create_task_executor.go:201–206):

  • The assignee validation block (if fm.Assignee() == "" { return error }) was removed — correct.
  • status validation remains — correct (still required).
  • The new 9-line comment is well-written and cites the real incident (bborbe/git-sync#5).

Test inversion (task_create_task_executor_test.go:129–146):

  • The "missing assignee" context is renamed to "empty assignee in frontmatter (task born parked)" — accurately describes the scenario.
  • Test now asserts err.NotTo(HaveOccurred()) and CallCount == 1 — correct for the new behavior.
  • Frontmatter uses status: todo, phase: human_review — matches the github-pr-watcher untrusted-author path described in the PR and the new code comment.

CHANGELOG: Clear, accurate, cites the incident and the doctrine rationale.

No issues found in the PR's actual changes.


Verdict

Must Fix (Critical): None.

Should Fix (Important): None — no issues in PR-touching code.

Nice to Have (Optional): None.

Selector clean — no adjudication needed.


{
"verdict": "approve",
"summary": "PR #14 is a clean, targeted bugfix that removes the overly-strict assignee validation and replaces it with a well-documented comment explaining the operator-inbox doctrine. The only code change is removing 3 lines of validation and adding 9 lines of comment; the test is correctly inverted to assert acceptance rather than rejection. No new violations introduced. All mechanical funnel findings are pre-existing in untouched code.",
"comments": [],
"concerns_addressed": [
"correctness: empty assignee correctly accepted — validateCreateTaskFrontmatter no longer requires assignee; status remains required; vault_scanner already treats empty assignee as unclaimed",
"correctness: status field remains required — unchanged validation; no other required fields were inadvertently modified",
"tests: test inverted from rejection to acceptance with correct frontmatter (status:todo, phase:human_review) matching the github-pr-watcher untrusted-author path"
]
}

@bborbe
bborbe merged commit 231f9a6 into masterJul 30, 2026
1 check passed
@bborbe
bborbe deleted the fix/allow-empty-assignee-on-create branch July 30, 2026 19:04
Sign up for freeto 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.

1 participant

@bborbe