Engine-wide fixes: production failures, money-path correctness, and CI by mmcky · Pull Request #6 · QuantEcon/contractor-payments

mmcky · 2026-07-30T07:14:36Z

Engine-wide fixes, split out of the Phase 5 branch so they can land without the reimbursement feature. No reimbursement code is included — verified: the diff introduces no reference to receipts, fetch_receipts, or reimbursement config, and none of the Phase 5-only files exist here.

Several of these are live production bugs affecting the timesheet and invoice flows today.

Already failing in production

The audit comment can never post, and it takes the processed label with it. Submission issues are closed and locked when filed, and GitHub rejects comments on a locked issue. The evidence is on the test repo: merged PRs #19–#26 carry processed; #32 and #36 do not, and issue #35's thread ends at "closed and locked" with no audit comment. It went unnoticed for exactly the reason below — the run went red in a step nobody watched. post_comment now unlocks, comments, and re-locks in a finally.

Pinning a caller to a branch never pinned the code it runs. The engine checkout had no ref:, so actions/checkout took the default branch: a branch's pipeline definition ran main's scripts. Every branch test to date passed for the wrong reason. Fixed with an optional engine_ref input defaulting to main, so callers pinned at @main that omit it are unaffected.

No failure gating anywhere. Every step carried the implicit success(), so any failure silently skipped the ledger, the email, the audit comment and the label. There is now a failure() reporter that states plainly whether the fiscal host was emailed — recovery differs entirely on that point.

Money path

A resubmit could get the wrong submission approved and emailed. On the update path the engine only ever added, so correcting the period before merge left both submissions on the branch, and head -1 approved whichever sorted first — for a forward correction, the abandoned one. Unlike a partial failure this is internally coherent: email, PDF and ledger row all agree, they just describe the wrong submission.

purge_branch_artifacts() removes what the branch added, stage_and_commit stages with git add --all -- so the removals actually reach the commit, and the workflow now refuses to proceed when a merge carries more than one submission. mark_superseded_yaml no longer assumes the predecessor shares the revision's period.

Correctness

Fix	Effect
Header-row heuristic	`bad-date \| 2 \| oops` matched on the substring "date" and was silently skipped as a header — a malformed row became a false `/validate` success (PLAN §10). Whole-cell matching closes it.
PR body refresh	The description froze at the first submission while the YAML and PDF moved on — and that body is the approval decision surface.
JPY separators	`163300` in the PDF vs `163,300` everywhere else: four surfaces, two spellings, one submission.
Ledger rounding	`305.21999999999997` reached the committed audit record while every display path formatted on the way out.
Concurrent merges	The losing push was simply rejected, silently costing the email and audit comment. Now fetch, rebase, retry.
Parse-step crash	An engine crash was reported to the contractor as their mistake, then crashed the error-comment step on the errors.json a dead parser never wrote.

Enforcement

tests.yml runs pytest on every PR across 3.12 and 3.13, with Typst installed at the version derived from the pipeline workflows so CI cannot drift from production.

tests/test_workflows.py covers what unit tests cannot reach: every workflow parses, every run: block passes bash -n, and every steps.<id> reference resolves — that last one fails open at runtime, so a typo silently stops a guard guarding. It caught one immediately during this port. It also encodes the money-path invariant: the approval-email step must carry no always()/failure() bypass, and processed must stay success()-gated.

Tests 230 → 315, plus 4 skips for the non-reusable workflows.

Not included, deliberately

Phase 5 reimbursements stay on phase-5-reimbursement. The receipts design ran into GitHub-shaped problems that make it a poor fit — attachment filenames anonymised to UUIDs, receipts stored outside git on an auth-gated CDN, a credential to read them back that a contractor could exfiltrate, and no privacy-safe way to centralise claims. That decision is recorded on that branch.

A `/submit` on an issue whose PR is already open only ever *added*: `stage_and_commit` was handed only the paths it had just written, so nothing removed what the previous run of the same issue produced. The damaging case is a corrected period. The contractor files for the wrong month, corrects it, and re-submits; both submission YAMLs end up on the branch. `process-approved.yml` then picked one with `head -1` — lexical order — which for a forward correction is the claim the contractor abandoned. That claim gets stamped approved, written to the ledger and emailed, while the corrected one stays `pending` on main forever. Unlike a partial failure this is internally coherent: the email, the PDF and the ledger row all agree, they just describe the wrong submission. There is no anomaly for an admin to notice. Three changes: * purge_branch_artifacts() removes every engine artifact the branch added on top of the base branch before the resubmit writes new ones, scoped via `base...HEAD` so artifacts already merged — the original behind a -vN revision — are left alone, and narrowed to what this run will regenerate so `--skip-pdf` keeps the committed PDF; * stage_and_commit() stages with `git add --all -- <paths>` so removals reach the commit. Without this the purge is a placebo: a plain `git add` of a vanished file leaves the deletion unstaged; * process-approved.yml refuses to proceed when a merge carries more than one submission file rather than silently taking the first, so the safety net holds independently of the purge. Also fixes mark_superseded_yaml(), which assumed the predecessor shares the revision's period. A revision that also corrects the period left it under the old one, so the lookup raised FileNotFoundError in the first post-merge step — which silently skipped the ledger update, the approval email and the audit comment while the revision was already stamped approved. It now falls back to scanning, and refuses to guess when the id is genuinely ambiguous. Tests 230 -> 244. The new cases drive real git repositories rather than asserting on mocked `git add` calls, because the defect lived in the gap between the filesystem writes and what actually got staged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Neither workflow had a single `always()`, `failure()` or `continue-on-error`, so every step carried GitHub's implicit `success()` and ANY failure silently skipped everything downstream — in process-approved that meant the ledger, the commit-back, the pinned issue, the approval email, the audit comment and the `processed` label. The audit comment could never post at all. Submission issues are closed *and locked* when filed, and GitHub rejects comments on a locked issue: GraphQL: Unable to create comment because issue is locked (addComment) This is live today, not hypothetical. On test-contractor-payments merged PRs #19-#26 carry `processed`; #32 and #36 do not, and issue #35's thread ends at "closed and locked" with no audit comment. It went unnoticed for exactly the reason above: the run went red in a step nobody watched, and everything after it was skipped in silence. `post_comment` now detects the lock rejection specifically, unlocks, comments, and re-locks in a `finally` — an unlocked submission issue is a worse outcome than a missing comment — while any other failure still raises without touching the lock. Gating added: * the audit comment now posts whenever the things it *asserts* actually happened (submission located, stamped, ledgered, pushed), so an SMTP failure yields a comment reading "Email: not sent" rather than silence; * a final `failure()` step reports the per-step outcome on the PR and the issue, states plainly whether the fiscal host was emailed — recovery differs entirely on that point — and notes the `processed` label was withheld deliberately; * `processed` is now explicitly `success()`-gated so a future `continue-on-error` cannot make a failed run look done; * in process-submission the `submit` label is removed on `always()`, since it is the contractor's only retry handle, and a `failure()` comment tells them whether anything was filed. `Send approval email` keeps its implicit `success()` throughout — nothing here can email the fiscal host on a failed run, which was the one direction that mattered. Also splits engine crashes from data errors in the parse step. It discarded the parser's exit code and inferred everything from whether submission.json appeared, so a crash became `parse_ok=false`, told the contractor their text was wrong, and then crashed the error-comment step on the errors.json a dead parser never wrote. Tests 244 -> 251. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Pinning a contractor repo's caller to `@some-branch` pinned only the workflow YAML, not the scripts it drives. The engine checkout had no `ref:`, and `actions/checkout` defaults to the repository's default branch — so a branch's pipeline definition ran `main`'s Python. This has been true since the reusable workflows were introduced. It stays invisible because the YAML and the scripts only disagree once one of them gains something the other lacks, so every branch test to date passed for the wrong reason: it was exercising `main`. The ref has to come from the caller. There is no context value for it — `github.job_workflow_ref` and `github.job_workflow_sha` both read as EMPTY on a real run, and `github.workflow_ref`/`workflow_sha` describe the *calling* workflow in the contractor repo rather than the reusable one. All three reusable workflows therefore take an optional `engine_ref` input defaulting to `main`, so a caller pinned at `@main` that omits it behaves exactly as before and the live contractor repos are unaffected. The cost of this shape is that a caller pins the engine twice — on the `uses:` line and in `engine_ref` — and the two can drift. That drift is what made the original bug undetectable, so it is now a test: contractor-template's callers pass `engine_ref` matching their `@ref`, and tests/test_workflows.py asserts they agree, that every reusable workflow declares the input, and that it stays optional with a `main` default. tests/test_workflows.py also covers what unit tests cannot reach: every workflow parses, every `run:` block passes `bash -n`, and every `steps.<id>` reference resolves to a real id — that last one fails *open* at runtime, so a typo silently stops a guard guarding. It caught one immediately: the failure reporter referenced `steps.create_pr` on a step that had no id. It further encodes the money-path invariant, asserting the approval-email step carries no `always()`/`failure()` bypass and that `processed` stays success-gated. Tests 251 -> 286 passing (2 skipped: the non-reusable workflows). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

**PR body refresh.** A push updates a PR's commits, never its body, so on the resubmit path the description kept describing the *first* submission — stale total, stale period, stale line items — while the YAML and PDF underneath it moved on. That body is the admin's approval decision surface, and "the admin reviews the PR body" is invoked as a mitigation elsewhere, so a stale one quietly undermines it. If the refresh fails the caveat is posted as a PR comment rather than left in a log nobody opens: the run stays green either way, so a warning on stderr is invisible to the person who matters. **Header-row heuristic.** The old rule tested whether the first cell *contained* one of "date", "day", "when", "id", "milestone". So `bad-date | 2 | oops` matched on the substring, failed `_parse_date`, and was silently skipped as a header — turning a malformed data row into a false `/validate` success (PLAN §10, E2E finding 2026-05-19). Matching whole cells closes that: `bad-date` contains "date" but is not equal to it. Requiring the first cell plus one other to be labels — rather than every cell — keeps headers recognised when a trailing column is renamed (`Day | Hours | Notes`) or emphasised (`**Date** | **Hours** | ...`). **JPY thousands separators.** `fmt_money` rendered JPY as `163300` while the email, the pinned ledger issue and the audit comment all said `163,300` — four surfaces, two spellings, for one submission. JPY has no minor units, which is why the branch exists, but it does use separators. **Ledger float artefacts.** Summing floats wrote `305.21999999999997` into the committed ledger while every display path formatted on the way out, so the stored audit record disagreed with every number anyone read. `round` on an int is a no-op, so JPY ledgers are unchanged. **Concurrent-merge push retry.** Two approvals merging close together both push to main; the loser was simply rejected, and with nothing failure-gated that silently cost the email, the audit comment and the label. Now fetch, rebase and retry, with a conflicting rebase stopping the run loudly rather than half-completing. Tests 288 -> 312. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The suite had no automation — tests that ran only when someone remembered to run them locally, so none of it was enforced on a PR and every fix in this branch protected nothing by default. `tests.yml` runs pytest on pull_request, on pushes to main, and on demand, across Python 3.12 and 3.13. 3.12 is what the submission and approval workflows run — the version that actually matters — and 3.13 is what gets used locally; testing both catches the drift where a fix verified on one breaks the other. Typst is installed at the version the pipeline pins, and the version is *derived* from process-submission.yml rather than repeated, so CI cannot drift from what renders contractors' documents. pyproject.toml gains an explicit package list. Without it setuptools' auto-discovery errors on the repo's several non-package top-level directories, so `pip install -e ".[dev]"` failed outright — found by running CI's own recipe in a clean venv rather than assuming it worked. Declaring the two real packages keeps the dependency list as the single source of truth instead of restating it in the workflow. Verified before pushing: a clean venv with only the declared dependencies installs and runs the full suite green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The code is complete, reviewed and tested; the decision is about fit. GitHub suits timesheets and invoices, where the data is the deliverable and the engine computes and validates against a contract. Reimbursements invert that — the evidence is the deliverable, and receipts are exactly what GitHub handles worst. notes/PHASE5-SHELVED.md records the measured reasons rather than the impressions: image attachments anonymised to UUIDs, no link between a receipt and a line item (entries sort by date, receipts by drag order), `GITHUB_TOKEN` returning 404 on user-attachments where a user PAT returns 200, that credential being exfiltratable by any contractor with Write, and no privacy-safe way to centralise claims. It also records what would justify revisiting — volume, not the credential — and the two cheaper experiments that would change the calculus. The engine-wide fixes found while building this branch are split onto `engine-fixes` (PR #6), so nothing of value is stranded here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR addresses production-impacting issues across the engine’s GitHub Actions workflows and the Python scripts they drive, with a focus on correctness in the submission/approval “money path”, reliable failure reporting, and enforceable CI to prevent workflow drift.

Changes:

Adds CI enforcement via a new Tests workflow and static workflow validation tests (tests/test_workflows.py) to catch YAML/shell syntax and invalid steps.<id> references before runtime.
Fixes workflow execution correctness and observability: pin engine checkouts to engine_ref, add explicit failure reporting, ensure retry safety for concurrent pushes, and prevent accidental approval processing when multiple submissions are present.
Fixes core script correctness around resubmits, header parsing, ledger rounding, JPY formatting, superseded-claim lookup, and posting audit comments to locked issues.

Reviewed changes

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

Show a summary per file

File	Description
`tests/test_workflows.py`	Adds static tests to validate workflow YAML, shell `run:` syntax, step-id references, Typst pin consistency, and engine checkout pinning.
`tests/test_update_ledger.py`	Adds regression tests ensuring ledger totals are rounded and JPY integer totals remain unchanged.
`tests/test_resubmit_artifacts.py`	Adds regression tests for resubmit behavior to ensure stale artifacts are purged and deletions are committed.
`tests/test_parse_issue.py`	Adds tests covering the tightened header heuristic and hand-typed header variants.
`tests/test_notify_comment.py`	Adds tests for comment rendering and locked-issue handling in `post_comment`.
`tests/test_finalize_approval.py`	Adds tests for robust superseded-YAML lookup across periods.
`scripts/update_ledger.py`	Rounds monetary totals to avoid float artefacts in committed ledger YAML.
`scripts/parse_issue.py`	Replaces substring-based header detection with exact-cell matching plus variant support.
`scripts/notify_comment.py`	Adds unlock/comment/re-lock behavior to successfully post audit comments to locked issues.
`scripts/generate_pdf.py`	Adds thousands separators for JPY amounts in the rendered PDF.
`scripts/finalize_approval.py`	Makes superseded submission lookup resilient when revisions also change period (with ambiguity safeguards).
`scripts/create_submission_pr.py`	Purges branch-added artifacts on resubmit, stages deletions reliably, and refreshes PR bodies (with fallback warning comment).
`pyproject.toml`	Adds explicit setuptools package list for editable installs.
`contractor-template/.github/workflows/process-approved.yml`	Pins `engine_ref` in the template caller workflows to match the `uses:@ref`.
`contractor-template/.github/workflows/period-reminders.yml`	Pins `engine_ref` in the template caller workflows to match the `uses:@ref`.
`contractor-template/.github/workflows/issue-to-pr.yml`	Pins `engine_ref` in the template caller workflows to match the `uses:@ref`.
`.github/workflows/tests.yml`	Adds a CI workflow running pytest across Python 3.12/3.13 and installs Typst pinned to workflow version.
`.github/workflows/send-reminders.yml`	Adds `engine_ref` input and pins engine checkout ref to it.
`.github/workflows/process-submission.yml`	Adds `engine_ref`, fixes parse-step crash semantics, adds true-always label removal, and adds failure reporting.
`.github/workflows/process-approved.yml`	Adds `engine_ref`, prevents multi-submission merges from being processed, retries push contention, and adds explicit failure reporting/gating.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+# The engine runs from a checkout (`python -m scripts.x`) and never installs
+# itself, so this repo has several top-level directories that are not packages
+# (docs/, templates/, notes/, contractors/, ...). Naming the two that are keeps
+# setuptools' auto-discovery from erroring out on the ambiguity, which is what
+# makes `pip install -e ".[dev]"` work for CI and for contributors — so the
+# dependency list above stays the single source of truth.
+[tool.setuptools]
+packages = ["scripts", "onboarding"]


mmcky · 2026-07-30T07:21:50Z

Tracking issue for the review handover: #7 — it lists what to check first, the follow-up steps (repointing the test repo callers, the live E2E), and what has not been verified.

mmcky and others added 5 commits July 30, 2026 16:59

Copilot AI review requested due to automatic review settings July 30, 2026 07:14

Copilot started reviewing on behalf of mmcky July 30, 2026 07:15 View session

mmcky mentioned this pull request Jul 30, 2026

Phase 5: Reimbursement Claim engine #4

Draft

4 tasks

Copilot AI reviewed Jul 30, 2026

View reviewed changes

mmcky mentioned this pull request Jul 30, 2026

Handover: review PR #6 (engine fixes), repoint test repo, Phase 5 parked #7

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Engine-wide fixes: production failures, money-path correctness, and CI - #6

Engine-wide fixes: production failures, money-path correctness, and CI#6
mmcky wants to merge 5 commits into
mainfrom
engine-fixes

mmcky commented Jul 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

mmcky commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mmcky commented Jul 30, 2026

Already failing in production

Money path

Correctness

Enforcement

Not included, deliberately

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mmcky commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants