Skip to content

Parse the Victorian general order and add Kanon fallback - #4

Closed
daviddiviny wants to merge 6 commits into
mainfrom
codex/check-parsing-progress
Closed

daviddiviny wants to merge 6 commits into
mainfrom
codex/check-parsing-progress

Conversation

@daviddiviny

@daviddiviny daviddiviny commented Apr 6, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • separate ministerial offices from officeholders so legal responsibility can attach to the office rather than the current person
  • add a deterministic parser for the Victorian General Order effective 30 October 2025, including shared responsibility, carve-outs, and residual administration rules
  • add an optional Kanon 2 assist pass for rules the deterministic parser still marks as partial, while preserving the base parse and original wording
  • persist the General Order snapshot into Postgres through a dedicated act_administration_rules table, a shared DB client, and a new worker sync task
  • add the first committed Drizzle migration for the repo schema and focused DB tests for the flattened persistence rows

Testing

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • DATABASE_URL=postgresql://postgres:postgres@localhost:54329/govgraph pnpm --filter @govgraph/db exec drizzle-kit migrate
  • DATABASE_URL=postgresql://postgres:postgres@localhost:54329/govgraph infisical run --env=dev -- pnpm exec tsx -e "import { createDbClient, closeDbClient, persistGeneralOrderDataset } from './packages/db/src/index.ts'; import { fetchVicGovGeneralOrder } from './packages/parsers/src/general-order/vicgov.ts'; void (async () => { const snapshot = await fetchVicGovGeneralOrder({ kanonAssistance: true }); const client = createDbClient(process.env.DATABASE_URL); try { const result = await persistGeneralOrderDataset(client.db, snapshot); const ruleCount = await client.db.execute('select count(*)::int as count from act_administration_rules'); const sourceCount = await client.db.execute('select count(*)::int as count from source_documents where source_url = \'https://www.vic.gov.au/general-order-effective-30-october-2025\''); const assistedCount = await client.db.execute('select count(*)::int as count from act_administration_rules where kanon_assistance is not null'); console.log(JSON.stringify({ result, persistedRuleCount: ruleCount.rows?.[0]?.count ?? null, sourceDocumentCount: sourceCount.rows?.[0]?.count ?? null, assistedRuleCount: assistedCount.rows?.[0]?.count ?? null }, null, 2)); } finally { await closeDbClient(client); } })();"

Risks

  • the Kanon-assisted path remains additive only; it decorates unresolved rules instead of replacing the deterministic parse
  • this repo now has a real DB client and initial migration baseline, so future schema changes should continue through committed Drizzle migrations
  • the worker uses DATABASE_URL if present and otherwise falls back to the local development Postgres default

Screenshots

  • not applicable

@daviddiviny

Copy link
Copy Markdown
Owner Author

@claude - review this PR

@claude

claude Bot commented Apr 7, 2026 •

Copy link
Copy Markdown

Claude finished @daviddiviny's task in 2m 15s —— View job


PR Review: Parse the Victorian General Order + Kanon Fallback

  • Gather context and understand the request
  • Read CLAUDE.md for repo guidelines
  • Review new parser files (vicgov.ts, kanon.ts, shared.ts)
  • Review schema changes (schemas.ts)
  • Review domain changes (catalog.ts, node-types.ts, edge-types.ts)
  • Review test coverage
  • Review fixture
  • Synthesize and post review

Overall Assessment

The design is sound. The rename from portfolio → ministerial_office is clean and consistently applied across all layers. The deterministic parser is well-structured with proper handling of the four heading styles, and the Kanon assist path is correctly additive-only. Below are issues worth resolving before merge, in order of severity.


Critical

1. DB enum rename has no migration

packages/db/src/schema/enums.ts renames "portfolio" → "ministerial_office" and "HOLDS_PORTFOLIO" → "HOLDS_OFFICE", but there are no Drizzle migration files in packages/db/. Postgres enum values cannot be renamed in-place — this requires ALTER TYPE ... RENAME VALUE or a recreate migration. If the DB is live, deploying these changes without a migration will either fail or leave the DB enum out of sync with the schema, breaking inserts and queries.

Fix this →


Important

2. Fixture loader exported in the production public API

packages/parsers/src/index.ts re-exports everything from ./general-order/vicgov, including loadFixtureVicGovGeneralOrder. Fixture helpers are test infrastructure and shouldn't be part of the package's public surface. Downstream consumers of @govgraph/parsers will see this function.

// packages/parsers/src/index.ts
export * from "./general-order/vicgov";  // exports loadFixtureVicGovGeneralOrder

Either export only the needed symbols with named exports, or move loadFixtureVicGovGeneralOrder to the test file that uses it.

Fix this →

3. "review" parse status is schema-declared but never produced

generalOrderParseStatusSchema declares three values — "parsed", "partial", and "review" — but the parser only ever emits "parsed" or "partial". If "review" is reserved for future human-triage workflows, a comment saying so would prevent confusion. If it's unused, remove it to keep the schema honest.

// packages/domain/src/government/schemas.ts:109
export const generalOrderParseStatusSchema = z.enum([
  "parsed",
  "partial",
  "review",  // never assigned by vicgov.ts or kanon.ts
]);

Minor

4. normalizeText wrapper in vicgov.ts is dead indirection

// packages/parsers/src/general-order/vicgov.ts:43
function normalizeText(value: string): string {
  return normalizeGeneralOrderText(value);
}

This adds a call-stack hop with no benefit. Call normalizeGeneralOrderText directly, or alias it at the import site with import { normalizeGeneralOrderText as normalizeText }.

5. batchSize cap of 8 is implicit

// packages/parsers/src/general-order/kanon.ts:222
const batchSize = Math.max(1, Math.min(options.batchSize ?? DEFAULT_BATCH_SIZE, 8));

The 8 magic number and why it's the ceiling (Isaacus API limit?) is not documented here or in AssistGeneralOrderWithKanonOptions. At minimum, the type comment on batchSize should note the accepted range.

6. extractOfficeNames has a narrow title pattern

The regex only recognises Attorney-General, Premier, Treasurer, and Minister for. "Deputy Premier" and "Minister without Portfolio" would fall through to the fallback normalisation. This is an explicit sprint-1 scope decision, but a // TODO comment would prevent a future contributor from being surprised when a new office title silently lands in the fallback branch.


What's working well

  • Correctness of the deterministic parser — the handling of except, scoped_list, and residual heading styles with finalizeActEntry is clean and the boundary between parsed/partial is properly conservative.
  • codePointSlice — using codepoint indexing rather than UTF-16 substring for Isaacus span offsets is the right call and avoids subtle bugs with multi-unit characters.
  • Kanon is strictly additive — the original rawText, parseStatus, and unparsedTail are preserved; enrichment only adds kanonAssistance. This is the correct design for a legal data pipeline.
  • Test coverage — the three Kanon scenarios (no client, enriched, failed) and the fixture-based parser tests cover the meaningful paths well.
  • structuredClone for immutability — clean deep-copy before mutation in the Kanon path.

@daviddiviny

Copy link
Copy Markdown
Owner Author

Follow-up changes are in now:

  • added the committed Drizzle migration for the new database shape
  • stopped exporting the fixture-only general order loader from the public parser entrypoint
  • removed the unused review parse status from the shared schema and database enum
  • cleaned up the parser helper aliasing and documented the Kanon batch-size guard

I re-ran pnpm lint, pnpm typecheck, and pnpm test, and they all passed on the updated branch.

@claude is your feedback properly addressed?

@claude

claude Bot commented Apr 7, 2026 •

Copy link
Copy Markdown

Claude finished @daviddiviny's task in 1m 37s —— View job


Follow-up Review: Verifying Addressed Feedback

  • Gather context
  • Verify: Drizzle migration for DB enum rename
  • Verify: Fixture loader removed from public API
  • Verify: "review" parse status removed
  • Verify: normalizeText alias cleaned up
  • Verify: batchSize cap documented
  • Synthesize and post final review

Verdict: Almost there — one migration/schema mismatch remains

Five of the six items are cleanly resolved. One issue slipped through.


Still needs fixing

"review" removed from schema but still in the migration file

The TypeScript side is correct — generalOrderParseStatusSchema in schemas.ts and generalOrderParseStatusEnum in enums.ts both now only declare "parsed" and "partial". But the initial migration file still creates the enum with all three values:

-- packages/db/drizzle/0000_brown_serpent_society.sql:9
CREATE TYPE "public"."general_order_parse_status" AS ENUM('parsed', 'partial', 'review');

If that migration is applied to a fresh DB, review becomes a valid enum value that the ORM schema doesn't know about. The snapshot at packages/db/drizzle/meta/0000_snapshot.json almost certainly also contains review for the same reason.

Since this is still a baseline migration with no live data, the fix is simple — regenerate the migration after the cleanup (or manually remove 'review' from both the .sql and the snapshot JSON), then re-run drizzle-kit migrate locally to verify.

Fix this →


Confirmed resolved

Item Status
DB enum rename has a migration ✅ The 0000 baseline creates ministerial_office and HOLDS_OFFICE directly — correct for a fresh schema
loadFixtureVicGovGeneralOrder out of public API ✅ packages/parsers/src/index.ts now uses named exports, fixture helper excluded
"review" removed from Zod + Drizzle TypeScript schemas ✅ Both schemas.ts and enums.ts are clean
normalizeText dead indirection ✅ Now imported as import { normalizeGeneralOrderText as normalizeText }
batchSize ceiling documented ✅ Comment at line 30: "Isaacus enrichment requests accept at most 8 texts per call."
extractOfficeNames TODO comment ✅ Added at line 137–138 noting Deputy Premier / Minister without Portfolio gap

@daviddiviny
daviddiviny force-pushed the codex/check-parsing-progress branch from ce194ae to 36083ea Compare April 7, 2026 09:01

Copy link
Copy Markdown
Owner Author

Addressed the remaining migration mismatch.

What changed:

  • removed review from packages/db/drizzle/0000_brown_serpent_society.sql
  • removed review from packages/db/drizzle/meta/0000_snapshot.json

What I verified locally:

  • pnpm exec drizzle-kit check --config packages/db/drizzle.config.ts
  • pnpm typecheck
  • pnpm test
  • applied migrations into a fresh local Postgres database from packages/db with pnpm exec drizzle-kit migrate --config drizzle.config.ts
  • confirmed the fresh DB enum now contains only parsed and partial

The preview app is still running locally on port 3020 against the imported General Order data.

@daviddiviny daviddiviny closed this Apr 7, 2026
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.

1 participant