fix(agent): fill blank fields on the dispatch tick instead of sign-in - #117
Merged
Conversation
…ase` Updated the release workflow to ensure that all merges into the `release` branch are done via pull requests, adhering to the established ruleset. This change prevents direct pushes that could lead to untracked releases and ensures that the release process is streamlined and error-free. The workflow now automatically creates and merges a pull request from `main` to `release` when a new tag is created, improving the reliability of the deployment process.
…ts into dispatch schedule This commit deletes the BlankFactsService and its associated methods, moving the functionality to the dispatch schedule. The `sweepBlankFacts` function is now called directly within the dispatch process, ensuring that blank fields are filled automatically at regular intervals. Additionally, the API documentation has been updated to reflect these changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/release.yml">
<violation number="1" location=".github/workflows/release.yml:55">
P3: If a prior run's merge attempts all failed, its `main -> release` PR stays open, and this list query will reuse that stale PR on the next run without refreshing its title/body to the current tag. Since head is always `main`, the merge still ships current main, but the merged PR title and the branch history keep pointing at the older tag while the step summary claims the new release shipped through it. Consider updating the reused PR's title/body to the current tag (or only reusing it when its title matches) so the shipped PR reflects the release actually being promoted.</violation>
<violation number="2" location=".github/workflows/release.yml:58">
P1: A commit landing on `main` after `$tag` is cut can enter this mutable-head PR before `gh pr merge`, so `release` can deploy untagged/unreleased code. Promote an immutable ref at the tagged SHA (and preserve that head from `pr-base.yml` retargeting) rather than `main` directly.</violation>
<violation number="3" location=".github/workflows/release.yml:61">
P2: The PR number is recovered with a second `gh pr list` after `gh pr create` succeeds. Because GitHub's list/search endpoints can be eventually consistent, that follow-up query can return empty right after creation, leaving `number` empty so `gh pr merge ""` fails all five retries and the run exits 1 while an open PR was actually created. Prefer capturing the number from `gh pr create`'s own output (e.g. `--json number --jq .number`), which is returned synchronously, instead of re-querying.</violation>
</file>
<file name="apps/agent/agent/schedules/dispatch.ts">
<violation number="1" location="apps/agent/agent/schedules/dispatch.ts:16">
P2: The fact sweep now runs on the every-minute dispatch cron, but unlike the rest of the agent's DB readers it takes no row locks to keep overlapping runs from stepping on each other. Every other queue consumer in this codebase guards its work (e.g. claimDue uses `FOR UPDATE SKIP LOCKED` specifically so two dispatchers take disjoint work; each sweep run is also awaited via waitUntil, so one that outlives the 60s cadence can overlap the next tick). Under overlap, two runs can read the same PROPOSED rows and whichever commits second will supersede the other's just-APPLIED fact. Consider aligning sweepBlankFacts with the codebase convention by locking the fill rows (e.g. FOR UPDATE on the selected proposals, or excluding already-APPLIED rows within the transaction) now that it runs every minute rather than on occasional sign-in/manual triggers.</violation>
</file>
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:156">
P3: The new Releases section describes the PR-based flow, but the Shipping-a-change section above (lines 103-110 'the tagged commit lands on `release` on its own', and line 91 'the two release pull requests below') still describes the old direct-merge scheme, so the same file now contradicts itself. Update the diagram and the line-91 wording to match the new 'one release pull request opened and merged by the workflow' flow so contributors read a single consistent picture.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number // empty') | ||
|
|
||
| if [ -z "$number" ]; then | ||
| gh pr create --base release --head main \ |
Contributor
There was a problem hiding this comment.
P1: A commit landing on main after $tag is cut can enter this mutable-head PR before gh pr merge, so release can deploy untagged/unreleased code. Promote an immutable ref at the tagged SHA (and preserve that head from pr-base.yml retargeting) rather than main directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/release.yml, line 58:
<comment>A commit landing on `main` after `$tag` is cut can enter this mutable-head PR before `gh pr merge`, so `release` can deploy untagged/unreleased code. Promote an immutable ref at the tagged SHA (and preserve that head from `pr-base.yml` retargeting) rather than `main` directly.</comment>
<file context>
@@ -27,42 +27,59 @@ jobs:
+ number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number // empty')
+
+ if [ -z "$number" ]; then
+ gh pr create --base release --head main \
+ --title "release: $tag" \
+ --body "Puts [$tag]($url) on \`release\`, which is the branch production deploys from. Opened by the Release workflow."
</file context>
| gh pr create --base release --head main \ | ||
| --title "release: $tag" \ | ||
| --body "Puts [$tag]($url) on \`release\`, which is the branch production deploys from. Opened by the Release workflow." | ||
| number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number') |
Contributor
There was a problem hiding this comment.
P2: The PR number is recovered with a second gh pr list after gh pr create succeeds. Because GitHub's list/search endpoints can be eventually consistent, that follow-up query can return empty right after creation, leaving number empty so gh pr merge "" fails all five retries and the run exits 1 while an open PR was actually created. Prefer capturing the number from gh pr create's own output (e.g. --json number --jq .number), which is returned synchronously, instead of re-querying.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/release.yml, line 61:
<comment>The PR number is recovered with a second `gh pr list` after `gh pr create` succeeds. Because GitHub's list/search endpoints can be eventually consistent, that follow-up query can return empty right after creation, leaving `number` empty so `gh pr merge ""` fails all five retries and the run exits 1 while an open PR was actually created. Prefer capturing the number from `gh pr create`'s own output (e.g. `--json number --jq .number`), which is returned synchronously, instead of re-querying.</comment>
<file context>
@@ -27,42 +27,59 @@ jobs:
+ gh pr create --base release --head main \
+ --title "release: $tag" \
+ --body "Puts [$tag]($url) on \`release\`, which is the branch production deploys from. Opened by the Release workflow."
+ number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number')
+ fi
+
</file context>
| async run({ receive, waitUntil, appAuth }) { | ||
| waitUntil( | ||
| Promise.all([ | ||
| sweepBlankFacts(), |
Contributor
There was a problem hiding this comment.
P2: The fact sweep now runs on the every-minute dispatch cron, but unlike the rest of the agent's DB readers it takes no row locks to keep overlapping runs from stepping on each other. Every other queue consumer in this codebase guards its work (e.g. claimDue uses FOR UPDATE SKIP LOCKED specifically so two dispatchers take disjoint work; each sweep run is also awaited via waitUntil, so one that outlives the 60s cadence can overlap the next tick). Under overlap, two runs can read the same PROPOSED rows and whichever commits second will supersede the other's just-APPLIED fact. Consider aligning sweepBlankFacts with the codebase convention by locking the fill rows (e.g. FOR UPDATE on the selected proposals, or excluding already-APPLIED rows within the transaction) now that it runs every minute rather than on occasional sign-in/manual triggers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/agent/agent/schedules/dispatch.ts, line 16:
<comment>The fact sweep now runs on the every-minute dispatch cron, but unlike the rest of the agent's DB readers it takes no row locks to keep overlapping runs from stepping on each other. Every other queue consumer in this codebase guards its work (e.g. claimDue uses `FOR UPDATE SKIP LOCKED` specifically so two dispatchers take disjoint work; each sweep run is also awaited via waitUntil, so one that outlives the 60s cadence can overlap the next tick). Under overlap, two runs can read the same PROPOSED rows and whichever commits second will supersede the other's just-APPLIED fact. Consider aligning sweepBlankFacts with the codebase convention by locking the fill rows (e.g. FOR UPDATE on the selected proposals, or excluding already-APPLIED rows within the transaction) now that it runs every minute rather than on occasional sign-in/manual triggers.</comment>
<file context>
@@ -12,6 +13,8 @@ export default defineSchedule({
async run({ receive, waitUntil, appAuth }) {
waitUntil(
Promise.all([
+ sweepBlankFacts(),
+
drainAll((task) =>
</file context>
| exit 1 | ||
| ;; | ||
| esac | ||
| number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number // empty') |
Contributor
There was a problem hiding this comment.
P3: If a prior run's merge attempts all failed, its main -> release PR stays open, and this list query will reuse that stale PR on the next run without refreshing its title/body to the current tag. Since head is always main, the merge still ships current main, but the merged PR title and the branch history keep pointing at the older tag while the step summary claims the new release shipped through it. Consider updating the reused PR's title/body to the current tag (or only reusing it when its title matches) so the shipped PR reflects the release actually being promoted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/release.yml, line 55:
<comment>If a prior run's merge attempts all failed, its `main -> release` PR stays open, and this list query will reuse that stale PR on the next run without refreshing its title/body to the current tag. Since head is always `main`, the merge still ships current main, but the merged PR title and the branch history keep pointing at the older tag while the step summary claims the new release shipped through it. Consider updating the reused PR's title/body to the current tag (or only reusing it when its title matches) so the shipped PR reflects the release actually being promoted.</comment>
<file context>
@@ -27,42 +27,59 @@ jobs:
- exit 1
- ;;
- esac
+ number=$(gh pr list --base release --head main --state open --limit 1 --json number --jq '.[0].number // empty')
+
+ if [ -z "$number" ]; then
</file context>
| @@ -153,8 +153,17 @@ releasable lands. It accumulates every releasable commit, so a stack of merges i | |||
| rather than five, and the notes are readable *before* you decide to ship them. | |||
Contributor
There was a problem hiding this comment.
P3: The new Releases section describes the PR-based flow, but the Shipping-a-change section above (lines 103-110 'the tagged commit lands on release on its own', and line 91 'the two release pull requests below') still describes the old direct-merge scheme, so the same file now contradicts itself. Update the diagram and the line-91 wording to match the new 'one release pull request opened and merged by the workflow' flow so contributors read a single consistent picture.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CONTRIBUTING.md, line 156:
<comment>The new Releases section describes the PR-based flow, but the Shipping-a-change section above (lines 103-110 'the tagged commit lands on `release` on its own', and line 91 'the two release pull requests below') still describes the old direct-merge scheme, so the same file now contradicts itself. Update the diagram and the line-91 wording to match the new 'one release pull request opened and merged by the workflow' flow so contributors read a single consistent picture.</comment>
<file context>
@@ -153,8 +153,17 @@ releasable lands. It accumulates every releasable commit, so a stack of merges i
Merging it writes `CHANGELOG.md`, bumps the version, tags `v0.2.0`, publishes the GitHub Release
-— and then the workflow merges that exact tagged commit into `release`, which is what a deploy and
-a plain clone both point at. **Nothing else to merge, and no order to remember.**
+— and then the workflow opens a `release: v0.2.0` pull request from `main` into `release` and
+merges it, which is what a deploy and a plain clone both point at. **Nothing else to merge, and no
+order to remember**: that pull request opens and closes inside the same run, and you see it only in
</file context>
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.
Opened automatically when
lewis/agent-backfill-fixwas pushed.The title is written from the diff and rewritten as you push, because this is squashed onto
mainand the title becomes the commit subject and the changelog line. Retitle it yourself and it is yours — the automation stops touching it.Summary by cubic
Moves blank-field backfill to the agent’s dispatch tick and enforces merging into
releasevia PRs. This fixes missed deployments and keeps blank fields filled regularly without relying on sign-in.Bug Fixes
maintoreleasewhen a tag is cut (or onworkflow_dispatch), satisfying the ruleset and preventing silent “tag published but not shipped” failures.Refactors
sweepBlankFactsat the start ofagent/schedules/dispatch.tsto apply suggestions to empty fields on a minute cron.BlankFactsServiceandPOST /internal/crm/apply-blank-facts; updatesBackfillServicelogging and docs to reflect the dispatch-based sweep.Written for commit 1c85674. Summary will update on new commits.