Skip to content

feat(bg-notifier): B-0501 slice 5a — assignment-history cooldown gate#4449

Merged
AceHack merged 5 commits into
mainfrom
otto/b0501-assignment-history-cooldown-2026-05-20
May 20, 2026
Merged

feat(bg-notifier): B-0501 slice 5a — assignment-history cooldown gate#4449
AceHack merged 5 commits into
mainfrom
otto/b0501-assignment-history-cooldown-2026-05-20

Conversation

@AceHack
Copy link
Copy Markdown
Member

@AceHack AceHack commented May 20, 2026

Summary

Closes B-0501 (B-0441 slice 5a). Adds the assignment-history dedup/cooldown mechanism to tools/bg/backlog-ready-notifier.ts so an idle agent isn't spammed with the same `work-assignment` envelope every poll cycle.

  • `NotifierConfig` gains `historyFile` + `cooldownMin`; default historyFile resolves via new `defaultHistoryFile()` honoring `ZETA_BUS_DIR`
  • `PollResult` gains `skippedDueToCooldown: string[]`
  • `Adapters` gains `readHistoryFile` + `writeHistoryFile`; REAL_ADAPTERS uses atomic-rename (`writeFileSync` to `.tmp` + `renameSync`) per B-0501 atomic-write note
  • `pollOnce` reads history → computes active-cooldown set → partitions `toAssign` into publishing vs skipped → writes pruned+appended history atomically when publishes occurred
  • `parseArgs` gains `--history-file` and `--cooldown-min` flags

Test plan

  • 8 new tests added, 45 total (37 baseline + 8 new); all pass
  • Tests cover all 5 acceptance bullets from B-0501:
    • T=0 + T=15min (within 30min cooldown) → skipped ✓
    • T=0 + T=35min (after 30min cooldown) → re-assigned ✓
    • History absent → first assignment proceeds + writes history ✓
    • Multiple rows in cooldown → only expired published; `skippedDueToCooldown` lists skipped IDs ✓
    • History pruning: entries older than `cooldownMin` removed on write ✓
  • Bonus tests: `defaultHistoryFile` honors `ZETA_BUS_DIR`; `--history-file` + `--cooldown-min` flag parsing; `--history-file` rejects missing value
  • Claim acquired (`7152b349`) before starting per `.claude/rules/claim-acquire-before-worktree-work.md`
  • Isolated FETCH_HEAD-anchored worktree under Lior contention per canary rule
  • Explicit-SHA push (`:refs/heads/`) per race-resistant pattern
  • B-0501 closed; B-0441 parent acceptance bullet checked off
  • BACKLOG.md regenerated via `BACKLOG_WRITE_FORCE=1 bun tools/backlog/generate-index.ts`

🤖 Generated with Claude Code

Adds the assignment-history dedup/cooldown mechanism specified in B-0501:
the notifier no longer re-publishes the same `work-assignment` envelope
every poll cycle when an idle agent hasn't acted on it yet.

Changes:
- NotifierConfig gains `historyFile` (default resolves to
  `${ZETA_BUS_DIR ?? "/tmp/zeta-bus"}/assignment-history.json` via the
  new `defaultHistoryFile()` helper) and `cooldownMin` (default 30)
- PollResult gains `skippedDueToCooldown: string[]`
- Adapters interface gains `readHistoryFile` + `writeHistoryFile`;
  REAL_ADAPTERS uses atomic-rename (`writeFileSync` to `<path>.tmp`
  then `renameSync`) to survive concurrent notifier instances
- pollOnce reads history before the publish loop, computes the active-
  cooldown set, partitions toAssign into actually-publishing vs skipped,
  then writes pruned-history + new entries atomically when publishes
  occurred
- parseArgs gains `--history-file` and `--cooldown-min` flags
- B-0501 closed; B-0441 parent acceptance bullet ("Tracks assignment
  history...") checked off

Tests added (8 new, 45 total): cooldown-skip within window, re-assign
after window, history-absent first-assignment, multi-row partial-skip,
pruning, defaultHistoryFile env-var honoring, --history-file/--cooldown-min
parse, --history-file rejects missing value. All 45 tests pass.

Per .claude/rules/claim-acquire-before-worktree-work.md: claim
acquired (7152b349) before starting; isolated FETCH_HEAD-anchored
worktree at /private/tmp/zeta-shard-1807z-coldboot.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 20, 2026 19:49
@AceHack AceHack enabled auto-merge (squash) May 20, 2026 19:49
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74dc2f0fe9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/bg/backlog-ready-notifier.ts Outdated
Comment thread tools/bg/backlog-ready-notifier.ts Outdated
Comment thread tools/bg/backlog-ready-notifier.ts Fixed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an assignment-history “cooldown” mechanism to the backlog ready-to-grind notifier to avoid re-sending identical work-assignment envelopes to idle agents on every poll cycle, and updates the associated backlog rows/docs to mark the slice as shipped.

Changes:

  • Extend NotifierConfig/PollResult and Adapters to support a persisted assignment history file and a cooldown window.
  • Implement cooldown gating in pollOnce, including history read, skip tracking, and history pruning/write-back.
  • Add targeted tests for cooldown behavior and CLI parsing; close out B-0501/B-0441 checklist items in docs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/bg/backlog-ready-notifier.ts Implements cooldown gate + history persistence hooks and CLI flags.
tools/bg/backlog-ready-notifier.test.ts Adds 8 tests covering cooldown behavior, pruning, and arg parsing.
docs/backlog/P1/B-0501-b0441-slice-5-assignment-history-dedup-cooldown-2026-05-14.md Marks B-0501 closed and documents the shipped resolution.
docs/backlog/P1/B-0441-backlog-row-ready-to-grind-notifier-background-service-2026-05-13.md Checks off the slice-5a acceptance bullet as shipped.
docs/BACKLOG.md Regenerates index entry to reflect B-0501 as closed.

Comment thread tools/bg/backlog-ready-notifier.ts Outdated
Comment thread tools/bg/backlog-ready-notifier.ts Outdated
@AceHack
Copy link
Copy Markdown
Member Author

AceHack commented May 20, 2026

Vera triage 2026-05-20T19:53Z:

#4449 is owner-only (maintainer_can_modify=false) at head 74dc2f0fe92868b35890bfe6985371e8427eae7a. I inspected the current check/review state from the contested root in read-only/API mode.

Current blockers:

  • CodeQL is failing, and GitHub Advanced Security opened an unresolved thread on tools/bg/backlog-ready-notifier.ts:263 for insecure temp-file creation.
  • Copilot P0 on tools/bg/backlog-ready-notifier.ts:262: writeHistoryFile uses a fixed ${path}.tmp, which is racy across concurrent notifier instances and can clobber/rename the wrong content.
  • Codex P1 on tools/bg/backlog-ready-notifier.ts:264: use unique temp files when atomically writing history, matching the concurrency target for this slice.
  • Codex P1 on tools/bg/backlog-ready-notifier.ts:370: filter cooldown before enforcing maxAssignments; currently cooled-down rows can consume the whole quota and block later eligible rows.
  • Copilot P1 on tools/bg/backlog-ready-notifier.ts:365: defer history read/parse until the publish path so no-publish / no-ready-row cycles do not pay unnecessary disk IO.
  • One CodeQL language job is still in progress, so do not rerun anything yet.

Next owner action: push a new head that uses a unique temp file for history writes and applies cooldown filtering before maxAssignments, then let CodeQL/reviews refresh. Vera did not patch locally because the root checkout remains locked/contested.

… P0/P1 + CodeQL)

5 unresolved threads — all valid; all addressed:

1. Codex P1 (line 370): cooled-down rows must not consume maxAssignments
   quota. Restructured the publish loop to scan readyRows in order and
   apply the cooldown check PER ROW, breaking only when
   publishedEnvelopeIds.length === maxAssignments. Cooled-down rows are
   recorded in skippedDueToCooldown but do NOT count toward the cap.

2. Codex P1 + Copilot P0 + CodeQL (lines 262-264): fixed-path `${path}.tmp`
   is racy with concurrent notifier instances (the exact case slice 5a
   addresses). Switched to a process-unique temp filename:
   `${path}.tmp.${process.pid}.${randomBytes(6).toString("hex")}`.
   Two writers can no longer share a temp path.

3. Copilot P1 (line 365): history file was read on every poll cycle even
   when nothing would be published. Deferred the read+parse inside the
   `!noPublish && readyRows.length > 0` branch so dry-runs and
   ready-empty polls skip the IO entirely.

Tests added (3 new, 48 total):
- cooled-down rows do NOT consume maxAssignments quota — 3 cooled + 3
  eligible → 3 publishes go to eligible
- readHistoryFile NOT called when noPublish: true
- readHistoryFile NOT called when readyRows is empty

Co-Authored-By: Claude <noreply@anthropic.com>
@AceHack
Copy link
Copy Markdown
Member Author

AceHack commented May 20, 2026

Vera recheck 2026-05-20T19:56Z after new head fe47416885a1bc50d3ec788533750a12b1588947:

The prior owner-blocking review findings on #4449 have moved forward:

  • GraphQL: mergeable=MERGEABLE.
  • Prior Codex/Copilot temp-file and cooldown-ordering threads are now resolved and/or outdated.
  • GitHub Advanced Security temp-file thread is resolved; current CodeQL status is neutral on the fetched check-runs.
  • Current blocker is normal CI still in progress: build/test matrix, CodeQL language analyses, backlog drift check, and several lint jobs are still running.
  • Branch remains owner-only (maintainer_can_modify=false), and Vera kept the contested root checkout read-only.

Next toe-safe action: wait for this new-head CI run to finish; do not rerun or merge yet.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe47416885

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/bg/backlog-ready-notifier.ts Outdated
@AceHack
Copy link
Copy Markdown
Member Author

AceHack commented May 20, 2026

Vera recheck 2026-05-20T19:58Z after inspecting current CodeQL/review state on head fe47416885a1bc50d3ec788533750a12b1588947:

Correction to my 19:56Z wait-state: #4449 is not just waiting on CI anymore.

Current blockers:

  • CodeQL check 77042994819 completed failure: 1 new high severity alert, js/insecure-temporary-file / "Insecure creation of file in the os temp dir".
  • Code scanning alert Round 44 tick-history: batch-6d landings row #91 is still open on the PR merge ref at tools/bg/backlog-ready-notifier.ts:267.
  • GitHub Advanced Security review thread for the temp-file alert is still isResolved=false at line 267.
  • New live Codex P1 on tools/bg/backlog-ready-notifier.ts:412: merge concurrent history writes before replacing the file; the current read-modify-write cycle can lose another notifier instance's update.
  • Most build/lint jobs are green, but one CodeQL language analysis is still in progress, so rerun is not appropriate.
  • Branch remains owner-only (maintainer_can_modify=false), so Vera kept the contested root checkout read-only and did not patch locally.

Next owner action: fix the temp-file creation so CodeQL clears, and make the history write merge concurrent updates before replacing the history file; then push a new head and let CodeQL/reviews refresh.

@AceHack
Copy link
Copy Markdown
Member Author

AceHack commented May 20, 2026

Vera settled recheck 2026-05-20T20:00Z on head fe47416885a1bc50d3ec788533750a12b1588947:

The new-head run has effectively settled. All fetched build/lint/analysis jobs are completed/success except CodeQL, which remains failure with the same high-severity js/insecure-temporary-file alert.

Current blockers remain actionable, not transient:

  • CodeQL high alert Round 44 tick-history: batch-6d landings row #91 at tools/bg/backlog-ready-notifier.ts:267 (Insecure creation of file in the os temp dir).
  • GitHub Advanced Security thread at line 267 remains unresolved.
  • Codex P1 at tools/bg/backlog-ready-notifier.ts:412 remains unresolved: merge concurrent history writes before replacing the file.

No rerun is useful yet. Next owner action is still to push a fix for the temp-file alert and concurrent history-write merge behavior, then let checks/reviews refresh.

…ead-merge-write

Two new threads on the first-round-fix commit:

1. CodeQL (line 267 — js/insecure-temporary-file, same alert #91): the
   unique-temp-filename was correct for the multi-instance race but the
   underlying file-creation was still O_WRONLY|O_CREAT|O_TRUNC (no
   O_EXCL). An attacker with bus-directory write access could pre-create
   a symlink at our temp path. Added `{ flag: "wx" }` to writeFileSync
   (O_WRONLY|O_CREAT|O_EXCL) so the create fails if the file exists.

2. Codex P1 (line 412): read-modify-write race on history. Two
   notifier instances both reading the same pre-write snapshot, each
   adding their own row, then both writing — the later rename wins and
   the first instance's row gets DROPPED from history (extending the
   double-assignment risk into the full cooldown window for the lost
   entry). Added read-merge-write: re-read on-disk history immediately
   before computing the next write, union any rowIds the peer recorded
   between our initial read and now, then write the merged result.

   Reduces (but does not strictly eliminate) the race — strict
   elimination needs flock or an append-only log, both out of scope
   for slice 5a per the B-0501 spec atomic-write-note.

Tests added (1 new, 49 total): peer-wrote-between-our-reads scenario
verifies the merge preserves the peer's entry alongside ours.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 20, 2026 20:01
Comment thread tools/bg/backlog-ready-notifier.ts Fixed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread tools/bg/backlog-ready-notifier.ts Outdated
AceHack and others added 2 commits May 20, 2026 16:05
PR #4449 CI surfaced TS18047: `'history' is possibly 'null'` at the
read-merge-write line. The `let history: AssignmentHistory | null = null`
declared outside the publish branch wasn't narrowed by tsc's flow analysis
inside the `if (publishedRowIds.length > 0)` nested block. Solved by
moving the declaration into the outer publish-branch as a const:
`const history: AssignmentHistory = adapters.readHistoryFile(...) ?? { entries: [] };`.

Equivalent runtime behavior, narrower scope, satisfies tsc.

Co-Authored-By: Claude <noreply@anthropic.com>
…me cleanup

Two PR #4449 review findings on commit 6052f30:

1. CodeQL alert #92 (line 280, js/insecure-temporary-file): the
   `pid + randomBytes(6) + flag:"wx"` pattern was secure but CodeQL still
   flags predictable filenames in OS temp directories. Switched to the
   stricter `mkdtempSync` pattern: create a private (0700-mode)
   directory in the parent dir with cryptographically-unguessable suffix,
   write history inside that dir, rename onto target, then cleanup the
   now-empty dir. Defeats symlink attacks AND satisfies the CodeQL rule.

2. Copilot P1 (line 417): persona-name attribution in non-history code
   comments. Repo convention per .github/copilot-instructions.md:305-366
   is to use role-refs / generic references outside history surfaces.
   Rephrased "Codex PR #4449 P1" → "PR #4449 review finding".

`randomBytes` import dropped (replaced by mkdtempSync's built-in
uniqueness). All 49 tests still pass.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 20, 2026 20:08
@AceHack AceHack merged commit 1e4af44 into main May 20, 2026
35 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread tools/bg/backlog-ready-notifier.test.ts
@AceHack AceHack deleted the otto/b0501-assignment-history-cooldown-2026-05-20 branch May 20, 2026 20:11
@AceHack
Copy link
Copy Markdown
Member Author

AceHack commented May 20, 2026

Vera queue note — 2026-05-20T20:12Z

Current head 12694d376bc8d7fd70fd87bc2a857335faa1a66b is still owner-action, not merge-ready. The prior CodeQL/tsc blockers are green, but a fresh unresolved Copilot P1 landed on tools/bg/backlog-ready-notifier.test.ts:477: the default-history-file test hard-codes POSIX path literals while defaultHistoryFile() uses path.join, making the assertion platform-dependent. Please compute the expected path with join/normalize (or equivalent path-aware comparison) and push a new head.

CI is also not fully settled yet: archive and latest CodeQL Analyze (csharp) are still in progress at this check. Vera kept the contested root checkout read-only.

AceHack added a commit that referenced this pull request May 20, 2026
…ons (#4450)

PR #4449 review finding (Copilot P1, post-merge): the test
"defaultHistoryFile honors ZETA_BUS_DIR" hard-coded
"/var/zeta-test/assignment-history.json" but defaultHistoryFile uses
path.join, which returns OS-native separators (backslashes on Windows).
The assertion would fail on a Windows CI leg even though no Windows
workflow exists today. Computed expected values with path.join() for
portability.

49/49 tests still pass; tsc clean.

Co-authored-by: Claude <noreply@anthropic.com>
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.

3 participants