Skip to content

216: Notion Factory Tasks DB → intake manifest generator - #280

Open
agent-relay-code[bot] wants to merge 7 commits into
mainfrom
factory/216-agentworkforce-factory-97e60383
Open

216: Notion Factory Tasks DB → intake manifest generator#280
agent-relay-code[bot] wants to merge 7 commits into
mainfrom
factory/216-agentworkforce-factory-97e60383

Conversation

@agent-relay-code

@agent-relay-code agent-relay-code Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Deliverable

A generator that queries the Factory Tasks Notion database and emits the Factory intake manifest (the NotionIntakeManifest shape consumed by src/intake/notion.ts), so rows set to Ready for Agent become dispatchable Factory tasks.

Context

Mapping (DB row → bootstrap)

  • page / authorizedPageId ← the row's own Notion page id
  • include only rows with Status = Ready for Agent (→ status: "ready")
  • title ← Task · recipe ← Recipe · summary ← page body (the brief) · reason ← Reason
  • targetsRepo + Labels{repo, labels} OR Project Path + Node{projectPath, node}
  • Route labels merged into the issue labels

Definition of done

  • CLI/script emits a manifest that passes manifestSchema (zod) validation from the live DB.
  • Only Ready for Agent rows are included; re-runs are idempotent.
  • Round-trips through runNotionIntake to publish/refresh GitHub issues.
  • Tests cover both a repo-target row and a workspace-target (projectPath/node) row.

Safety gates

Do not merge; open a PR parked in human-review. Do not auto-flip any DB row's Status — humans move a row to Ready for Agent.


Recipe: single. Filed from the Factory Tasks Notion DB.

Fixes #216


Summary by cubic

Generates Factory intake manifests from the Factory Tasks Notion database and binds durable work‑unit claims to the provider‑native page identity. Previously each destination created its own claim; now claims use notion:, we migrate agreeing legacy destination claims, refuse migration and dispatch when legacy digests disagree, and still block dispatch if the mounted spec digest changes.

The new command factory intake notion generate emits a schema‑validated manifest in stable canonical page‑id order and never constructs a fleet. It selects only rows with Status = Ready for Agent, reads complete page markdown for the private summary, maps each ready row to exactly one target (repo with merged Labels + Route and optional Public Summary, or workspace via Project Path + optional Node), and fails closed on ambiguous/missing targets, missing required properties, truncated or wrong‑page markdown, or no ready rows. The read‑only Notion client uses NOTION_API_KEY, resolves the live data‑source schema to filter server‑side for Ready for Agent, paginates via next_cursor, and accepts collection:// overrides. Options: --data-source, --mount-root, --worker-mount-root, --worker-mount-transport (local | relay-channel), --state-path.

Durable claims now key work units by the provider identity and record destination deliveries separately. The system migrates existing destination claims discovered by page‑key prefix into the canonical notion: claim when digests agree; if multiple legacy claims disagree, it refuses dispatch and does not write a canonical claim. Claim discovery requires workspace‑global Relay channel listing.

Rollout

  • Provide a read‑only NOTION_API_KEY with access to the Factory Tasks data source; optionally set --data-source.
  • Generate and validate: factory intake notion generate > ./ops/notion-intake.json and parse with manifestSchema.
  • Ensure the Relay workspace key can list channels for claim discovery; if dispatch fails with “legacy Notion claims disagree…”, remove or reconcile conflicting legacy claim channels for that page before retrying.
  • Do not run non‑dry intake against production rows.

Written for commit 45053cc. Summary will update on new commits.

Review in cubic

@reviewsaur

reviewsaur Bot commented Aug 17, 2026

Copy link
Copy Markdown

🦕 Reviewsaur Quiz

A review comprehension quiz has been generated for this PR.

Take the quiz

Attempt 1 | (0/1 approval) | This link is unique to you and expires when the PR is closed.

Tip: To require this quiz before merging, enable it as a required status check.

@kjgbot

kjgbot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Independent review environment (before review work):

pwd: /Users/khaliqgant/Projects/AgentWorkforce/chief
hostname: SF-Mac-Mini
gh auth status: authenticated to github.com as kjgbot; active account; git protocol ssh; token scopes admin:public_key, gist, read:org, repo
node --version: v22.23.2

The spawn cwd is not /Users/khaliqgant/Projects/AgentWorkforce/factory; I will use explicit git -C /Users/khaliqgant/Projects/AgentWorkforce/factory paths and will not stash or alter the shared checkout.

@kjgbot

kjgbot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Independent review — changes requested

Reviewed head 26c6edc3 against freshly fetched origin/main 7122524. I used an isolated detached worktree; no branch changes were made or pushed.

What this PR actually does

The new NotionApiFactoryTasksClient reads the data-source descriptor, chooses a status or select filter from the live Status property type, POSTs the paginated Ready for Agent query, and retrieves each matching page through the markdown endpoint. generateFactoryTasksManifest() then rechecks readiness, canonicalizes/sorts page IDs, requires exactly one of Repo and Project Path, merges Labels plus Route, retrieves the page body as the bootstrap summary, and validates the complete output with the existing intake manifestSchema (src/intake/notion-manifest.ts:81-141,174-238). This matches the current Notion query and markdown API shapes: https://developers.notion.com/reference/query-a-data-source and https://developers.notion.com/guides/data-apis/working-with-markdown-content.

The CLI branch reads NOTION_API_KEY, forwards only the generator flags, prints the manifest, and returns 0 only after generation succeeds (src/cli/fleet.ts:219-234). Generation itself does not mount content, claim work, create an issue, or dispatch an agent. The consumer is runNotionIntake(): it rereads mountRoot/pages/<page>/content.md, derives its digest and claim key, claims before the side effect, writes receipts, and returns ok:false on per-task failure.

The generator does not read Factory config. It runs before loadConfig; --config is therefore irrelevant to this command. Omitted generator flags are not empty values: manifestSchema supplies .integrations/notion, local worker transport, and .factory/notion-intake-state.json. I found no second config-search or contract-combining path.

Findings

  1. HIGH — CONFIRMED: every generated repository task is blocked when the destination repository is public.

    The generator can only emit { repo, labels } (src/intake/notion-manifest.ts:199-205); it never supplies the consumer's optional publicSummary, and FACTORY_TASK_PROPERTIES has no source property for one. The consumer refuses every public repo target without that field before claiming or creating an issue (src/intake/notion.ts:513-516). The new round-trip test hides this by hard-coding repositoryVisibility() to private (src/intake/notion-manifest.test.ts:311-325), even though its repo fixture is AgentWorkforce/factory.

    Concrete scenario: a Ready row has Repo = AgentWorkforce/factory, valid Task/Reason/Recipe, and a complete page body. Generation succeeds. Intake sees the public repository and returns ok:false, status:"blocked", reason public repository requires an explicit publicSummary; createIssue is never called and the CLI exits 1. I confirmed this with a focused generator→consumer probe (exit 0 for the assertion that the task blocks and no issue is created). The primary repo-target deliverable therefore cannot complete for public repositories. Add an explicit reviewed public-summary source/mapping and test the actual public visibility branch; do not reuse the private page body.

  2. HIGH — CONFIRMED: the durable claim is keyed on the mutable destination, so one Notion work unit can be offered twice.

    The manifest correctly carries provider-native page identity in page and authorizedPageId, but the consumer turns it into notion:<page-id>:<repo-or-project-path> (src/intake/notion.ts:292-295). Repo and Project Path are mutable row properties. Changing either one creates a new source key, a new claim, and a new side effect instead of reconciling the existing work unit.

    Concrete scenario: generate and dispatch page 11111111-1111-4111-8111-111111111111 with Repo = Example/one; while it remains Ready, change only Repo to Example/two, regenerate, and dispatch again. My scratch probe produced two dispatched results, two issue creations, and two durable keys: notion:<page>:repo:example/one and notion:<page>:repo:example/two. This is a confirmed duplicate-offer path, not a hypothetical concurrency race. The claim authority needs an immutable work-unit identity; destination can remain reconciliation/alias data but cannot create a fresh authority for the same page.

  3. MEDIUM — CONFIRMED TEST GAP: three advertised fail-closed guards can be deleted while every new generator test remains green.

    These are currently implemented correctly, so this is a coverage finding rather than a claim that head already accepts the bad response. The mutation results are concrete:

    • deleting the truncated || unknown_block_ids.length rejection stayed GREEN (4/4);
    • deleting the response-page-ID equality check stayed GREEN (4/4);
    • deleting the duplicate-page rejection stayed GREEN (4/4).

    Failure scenarios are respectively: a partial brief is authorized as complete; markdown for page B is bound to page A's authorization; or one provider page is emitted twice. These are explicit safety properties in the implementation/README and need must-fire tests against NotionApiFactoryTasksClient/the generator.

Mutation battery

Each mutation was applied alone to production code in the scratch worktree, the new test file was run by exit code, and the mutation was then reverted.

Mutation Result
Remove client-side Ready-only filter RED, exit 1; Draft row entered manifest
Change server filter from Ready to Draft RED, exit 1
Disable pagination after page one RED, exit 1
Reverse stable page-ID order RED, exit 1
Drop Route labels RED, exit 1
Remove exactly-one-destination rejection RED, exit 1
Drop CLI worker transport propagation RED, exit 1
Make normalized source keys unstable between runs RED, exit 1; second run dispatched again
Accept truncated/unknown markdown GREEN, exit 0 (finding above)
Accept markdown for a different page ID GREEN, exit 0 (finding above)
Accept duplicate provider rows GREEN, exit 0 (finding above)

Final unmutated checks: src/intake/notion-manifest.test.ts 4/4 passed; the CLI parser test passed; npm run build exited 0. Existing focused tests also passed for (a) claim-write failure occurring before workspace spawn and (b) blocked intake causing CLI exit 1. A separate fake-client probe confirmed queryReadyTasks() -> [] throws instead of producing a silent empty manifest.

CI and safety audit

gh run list --repo AgentWorkforce/factory --branch factory/216-agentworkforce-factory-97e60383 returned one completed CI workflow for this exact head, conclusion success. Its five jobs all succeeded: package, verification-gate-e2e, verification-stack-e2e, load-e2e, and kubernetes-provider-e2e. The stale Reviewsaur Quiz status is not treated as CI.

Claim behavior remains fail-closed in the consumer: a failed claim write produces a blocked result before spawn/create, and the CLI maps report.ok === false to exit 1. Generator exceptions also reach the CLI catch and return 1. I found no new success exit for an unperformed action.

Secret audit: the Notion token is trimmed into a private field and used only to construct the Authorization header. It is not interpolated into emitted JSON, paths, or errors; the query body contains no token. The generator never reads the Relay workspace key. I scanned the diff for token/workspace-key/connection-string values and found none committed. API errors include method, endpoint path, status, and bounded server response text, but not request headers.

Residual UNKNOWN / not covered

  • Live Notion behavior is UNKNOWN. NOTION_API_KEY is absent here, so I did not query the real data source, compare live rows/statuses, or run the required empty-result bogus-input control against a deliberately fake data-source ID. The fake [] rejection proves only local behavior; it does not prove the live query returns the intended rows.
  • No live end-to-end dispatch. I did not create a real GitHub issue, spawn a real workspace agent, publish a real relay-channel contract, or exercise an actual Agent Relay durable claim.
  • No live mount-fidelity check. I did not compare Notion's markdown response with the Relayfile-mounted content.md consumed later.
  • No large-source boundary check. I did not exercise the Notion query endpoint's 10,000-result pagination ceiling.

Because findings 1 and 2 affect whether repo work can dispatch at all and whether the same work can dispatch twice, I do not consider this ready to merge.

@khaliqgant

Copy link
Copy Markdown
Member

Worker start report for conflict/high-defect remediation:

  • pwd: /Users/khaliqgant/Projects/AgentWorkforce/factory
  • hostname: Khaliqs-MacBook-Pro.local
  • gh account: khaliqgant (active; SSH git protocol)
  • gh scopes: admin:public_key, delete_repo, gist, read:org, repo, workflow
  • Node: v22.22.2

The requested Veto MCP commands are not exposed in this worker session, so I cannot run veto_status/veto_discover; I will use read-only repository inspection and the repository test/review gates as the fallback. No files have been touched.

@khaliqgant

Copy link
Copy Markdown
Member

Task 2 design decision (before implementation)

Choice: (c). The authority is one immutable provider-native Notion work unit (notion:<canonical-page-id>), while each declared target remains a destination-scoped delivery/reconciliation alias. A target is not a second work unit.

  • I will extend/reuse src/dispatch/work-unit-identity.ts from fix(factory): stamp dispatch work-unit identities #279 for the canonical Notion page identity. I will not derive authority from Repo, Project Path, a mount path, or a target-list position.
  • One canonical page claim authorizes the complete target fan-out in the exact manifest snapshot. Each target keeps a distinct sourceKey, result, receipt, issue marker/invocation identity, and duplicate-destination check. A destination-scoped delivery claim may still serialize the individual side effect, but it cannot authorize dispatch unless the immutable page claim exists and its digest matches.
  • Therefore one page with two declared targets can dispatch both under the same page authority, while two distinct pages receive distinct page authorities.

Mid-dispatch edit: the in-flight run is bound to the already-authorized manifest snapshot/digest and may finish the original destination. If the mounted snapshot changes, the existing byte/digest checks block it. Regenerating after a Notion Repo edit produces a different authorization digest but the same page authority, so the later run blocks loudly before claim/create/spawn; it cannot mint authority for the edited destination and cannot create a second issue.

Persisted-claim compatibility: migration/dual-read is required. I will not silently switch formats. When the stable page claim is absent, the claim store will discover legacy notion:<page>:<destination> claims, backfill the stable page claim from the authoritative legacy digest, and refuse conflicting/mismatched legacy digests. The current destination key and prior receipt aliases remain readable for reconciliation. Fresh writes create the stable page authority plus the destination-scoped delivery record. This covers a destination edit even when the old destination is no longer present in the regenerated manifest.

Required regression pair:

  1. Must-fire: dispatch a Ready generated page to repo A, edit only Repo to repo B, regenerate with independent local state, and assert the second report is blocked, issue creation remains exactly once, and only one canonical page authority exists.
  2. Must-not-fire: (a) two distinct page IDs each dispatch once, and (b) one page with two distinct declared targets dispatches both once under one canonical page authority, with separate per-target receipts/results.

I will merge current origin/main next, preserve #282 placement refusal and #274 exit semantics, then implement this shape.

# Conflicts:
#	src/__tests__/dist-entrypoints.test.ts
@khaliqgant

Copy link
Copy Markdown
Member

Conflict resolution and high-defect remediation complete

Pushed head e95216c to the PR branch. Current origin/main eaaa076 is an ancestor of the head; PR is now MERGEABLE.

Merge/seam result

HIGH 1: mutable destination duplicate

  • Claim authority is the immutable provider page identity and digest. Destination-shaped sourceKey values remain only per-target delivery/reconciliation records, receipts, markers, and invocation aliases.
  • One page authority can fan out to multiple declared targets; each target is still independently serialized and reconciled.
  • Legacy destination claims are not orphaned: the Relay store can discover them by provider-page prefix, the first authoritative legacy digest is backfilled into the stable page claim, conflicting legacy digests fail closed, and an edited destination with a changed digest is refused before create/spawn.
  • In-flight behavior matches the design comment: the authorized snapshot may finish its original target; a regenerated row with an edited Repo has the same page authority and a different digest, so it blocks before a second offer.

Regression evidence:

  • Must-fire green on the fix: Ready page -> Example/one, edit only Repo -> Example/two, independent local state: second report blocked, createIssue remains exactly 1, and no destination claim for repo two is written.
  • Negative control: in a detached scratch worktree I bypassed the page authority and ran only that test. It exited 1 and showed the edited repo as status: dispatched, reproducing the defect.
  • Must-not-fire: two distinct page IDs plus one page with two distinct targets exited 0; all three deliveries dispatched exactly once under two canonical page authorities.
  • Legacy migration test: an old destination claim is backfilled to notion:<page> and an edited destination is blocked before issue creation.

HIGH 2: generated public-repository tasks

  • Added the explicit reviewed Notion Public Summary property to the queried property set and mapped it only to repository targets as publicSummary.
  • The generator -> intake round trip now uses repositoryVisibility: public, asserts dispatch succeeds, asserts the reviewed public summary is in the issue, and asserts the private mounted brief is absent.
  • The existing refusal without a reviewed public summary remains intact.

I also added the three missing fail-closed tests from the independent review: truncated markdown, mismatched markdown page ID, and duplicate provider page rows.

Local verification by exit code

  • Focused: 5 files, 177 tests passed.
  • Full npm test: 92 files passed / 1 skipped; 1700 tests passed / 1 skipped; exit 0.
  • npm run build: exit 0.
  • npm run featuremap:check: ok: true, 319 features, no advisories; exit 0.
  • git diff --check: exit 0.
  • Review threads: 0 total / 0 unresolved.

CI workflow CI is now running for e95216c: https://github.com/AgentWorkforce/factory/actions/runs/32025289840 . I will stay online and report every workflow/job conclusion rather than treating an empty or aggregate result as success.

Veto MCP remained unavailable in this worker session, as noted in the start report, so veto_diff_review could not be invoked; the repository build, full suite, mutation check, feature-map gate, and GitHub CI are the available validation gates.

@khaliqgant

Copy link
Copy Markdown
Member

Final gate result

Head e95216c is MERGEABLE with merge state CLEAN. Review threads: 0 total / 0 unresolved.

Per-workflow branch query (gh run list --repo AgentWorkforce/factory --branch factory/216-agentworkforce-factory-97e60383) returned the current-head workflow:

Every CI job passed:

  • package — pass (4m24s)
  • kubernetes-provider-e2e — pass (2m44s)
  • load-e2e — pass (2m26s)
  • verification-gate-e2e — pass (2m47s)
  • verification-stack-e2e — pass (2m16s)

The isolated worktree is clean and matches the pushed PR head. I did not merge; the explicit gate ownership remains with Khaliq.

@khaliqgant

Copy link
Copy Markdown
Member

Both original findings verified FIXED. One new defect in the migration blocks merge — and it is a five-line reorder.

Verified at head e95216cd by reading the code, not by trusting the report:

  • Claim authority is now immutable. workUnitKey = dispatchNotionPageIdentity(pageId) (:288), and the claim is taken on it — input.claims.get(task.workUnitKey) (:715), claims.claim({ sourceKey: task.workUnitKey, ... }). The destination survives only as a delivery alias. That is exactly right, and using the shared dispatchNotionPageIdentity helper rather than a parallel scheme is what I wanted.
  • Legacy claims are migrated rather than orphaned (:721-726). You anticipated the concern I raised on [factory] Key dispatch claims on work-unit identity, not surface path #211 — changing a claim key format silently orphans every already-persisted claim — without being told.
  • Public repos work. The generator now reads a Public Summary Notion property (notion-manifest.ts:202,207), publicSummary is on the schema (:18), and notion-manifest.test.ts:480 stubs repositoryVisibility as public instead of hard-coding private to dodge the branch. Also good: :486 uses target.publicSummary for a public repo rather than reusing the private page body, so internal content cannot leak into a public issue.

THE BLOCKER — the ambiguity check runs AFTER the write it is supposed to prevent

In resolveNotionClaim, the order is:

  1. const [authoritative] = legacyClaims — arbitrary, earliest by claimedAt
  2. await input.claims.claim({ sourceKey: task.workUnitKey, digest: authoritative.digest, ... })writes the canonical claim
  3. assertNotionClaim(migrated.claim, task.workUnitKey, authoritative.digest)
  4. const legacyDigests = new Set(legacyClaims.map((claim) => claim.digest))
  5. if (legacyDigests.size > 1) throw new Error('legacy Notion claims disagree ... refusing dispatch')checks after

When legacy claims disagree, you have already persisted a canonical claim under workUnitKey carrying the arbitrarily-chosen earliest digest, and only then thrown.

Why the throw does not save you: the failure is fail-closed once, then fail-open forever. On the next run, input.claims.get(task.workUnitKey) at :715 finds the row you just wrote and returns early at :718 — the legacy branch, and therefore the ambiguity check, is never reached again. So a genuinely ambiguous migration gets silently resolved in favour of whichever claim happened to be earliest, on the second attempt, with no operator ever seeing the conflict.

That is the exact shape I ruled against on #211 an hour ago: "if more than one alias has a live unexpired lease, migration refuses to choose and dispatch remains blocked for operator reconciliation. It must not abandon possibly live work by guesswork." Here the guess becomes durable.

Fix: compute legacyDigests and throw before calling claims.claim. Nothing is written until the migration is unambiguous.

Tests required:

  • Must-fire: two legacy claims with disagreeing digests → refuses, and no canonical row is written. Assert the absence of the row, not just the throw — asserting only the exception is what let this through.
  • Must-fire, second run: after that refusal, a repeat run still refuses. This is the discriminating case and the one the current code fails.
  • Must-not-fire: two legacy claims that agree → migrates cleanly to one canonical claim.

One more thing, and it is why I read the code at all

This PR has zero reviews — the reviews array is empty and no cubic or CodeRabbit check is present, only the five CI jobs. That is the same condition that let the original two HIGH findings sit unnoticed this morning: 0 unresolved threads because nothing ever looked. Your fix is good work, but a claim-migration path merged with no independent review is not something I will put in front of the principal. Once the reorder lands, request a review explicitly and let a reviewer see the migration.

Do not treat green CI as the finish line here — CI passed on the version containing the defect above.

@khaliqgant
khaliqgant requested a review from barryollama August 17, 2026 14:03
@khaliqgant

Copy link
Copy Markdown
Member

@barryollama review requested on the hardened Notion claim-migration path. Please focus on ensureNotionWorkUnitClaim: conflicting legacy digests must be rejected before any canonical page claim is written, and a repeated run must remain fail-closed. The branch also merges current main and reconciles the combined feature catalog count.

Local verification: 4 focused seam files (50 tests), feature-map validation, build, full suite with two workers (1726 passed / 1 skipped), package dry-run, and a scoped TruffleHog diff scan with zero findings.

@khaliqgant

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
❌ Action failed

Review failed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Caution

Review failed

An error occurred during the review process. Please try again later.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

Notion Factory Tasks DB → intake manifest generator

2 participants