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

[mmcky]

Handover from the review + E2E session on 2026-07-30. Everything below is either committed or recorded on a branch — nothing is only in a chat log.

Start here

Review #6. It is ready, CI is green, and it fixes bugs that are breaking the production timesheet flow today. Read commit 22a4927 first — that is the one with the live failure.

PR	Branch	State	What
#6	engine-fixes	Ready, CI green (315 tests on 3.12 + 3.13)	Engine-wide fixes. This is the one to land.
#4	phase-5-reimbursement	Draft — parked by decision	Reimbursements. Complete and tested, deliberately not shipping.

Three bugs already failing in production

These affect timesheets and invoices. None of them involve reimbursements.

1. The post-merge audit comment can never post, and it silently takes the processed label with it. Submission issues are closed and locked when filed, and GitHub rejects comments on a locked issue. Evidence 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 precisely because nothing gated on failure — the run went red in a step nobody watches, and every step after it was skipped in silence.

2. Pinning a caller to a branch never pinned the code that branch runs. The engine checkout had no ref:, so actions/checkout took the engine's default branch: a branch's workflow YAML was driving main's scripts. The consequence worth absorbing is that every "E2E verified on a branch" result before today passed for the wrong reason — it was exercising main.

3. A resubmit could get the wrong submission approved and emailed. Correcting the claim period before merge left both submissions on the branch, and head -1 approved whichever sorted first — for a forward correction, the abandoned one. This is the nastiest of the three because it is internally coherent: the email, the PDF and the ledger row all agree with each other, they just describe a submission the contractor abandoned. There is no anomaly for an admin to spot.

Also in #6: failure gating across both workflows, the header heuristic that silently swallowed malformed rows (PLAN §10), stale PR bodies on resubmit, JPY thousands separators, ledger float artefacts reaching the committed audit record, concurrent-merge races, and engine crashes being reported to contractors as their own mistake. Plus CI, which did not exist, and tests/test_workflows.py, which caught a fail-open steps.<id> typo during the port itself.

Checklist

• Review and merge Engine-wide fixes: production failures, money-path correctness, and CI #6
• Repoint test-contractor-payments callers from @phase-5-reimbursement back to @main — change both the uses: ref and the engine_ref input, they must agree or the engine runs a different commit than the workflow
• Run one timesheet end-to-end on the test repo against merged main (Engine-wide fixes: production failures, money-path correctness, and CI #6 itself has had no live E2E — see below)
• Decide whether to make Tests a required status check on main
• Clean up leftover E2E state on the test repo: issues #39 and #41, submissions under 2026-03 and 2026-04, the throwaway spike-attachment-fetch.yml, and issue #38

Open decisions

Contractors hold Write (onboarding/new-contractor.py:970, permission=push), which is what makes org secrets exfiltratable — any contractor can add a workflow to their own repo that prints them. Zero contractors have ever pushed a commit to any of the three live repos, so dropping to Triage looks free. The blocker found in analysis: the engine locks submission issues, and locked issues reject comments, so a revision's /submit would have to go through the submit label instead. Not actioned.

CI is not a required check, so #6 can still merge red. Making it required is the deferred branch-protection work in PLAN §8 Phase 3b.

Why reimbursements are parked

GitHub fits timesheets and invoices because the data is the deliverable — hours times rate, milestones against a contract schedule, all computed and validated. Reimbursements invert that: the evidence is the deliverable, the data is a trivial sum, and receipts are exactly what GitHub handles worst. Each of the following was measured rather than assumed.

Problem	Evidence
Image attachments lose their filenames	GitHub anonymises them — receipt-taxi.png reaches the fiscal host as 03-55070d4e-6445-4df2-8efd-914dad928044.png
Nothing links a receipt to a line item	Entries sort by date, receipts number by drag order, so receipt 03 can back entry 1
The pipeline cannot fetch receipts	Stage 0b: GITHUB_TOKEN → 404, a user PAT → 200, no auth → 404
That credential is exfiltratable	The reusable workflow runs as a job in the contractor's own repo, where they hold Write
Claims cannot be centralised	A shared repo exposes every claimant's receipts to every other claimant, permanently, in git history

The cost lands on the fiscal host, who receives UUID-named attachments with no stated correspondence to the line items. Against roughly a dozen claims a year that did not justify ~1,700 lines plus a credential design plus the privacy question.

Full reasoning, what is on the branch, and what would justify revisiting are in notes/PHASE5-SHELVED.md on phase-5-reimbursement. The trigger to revisit is volume, not the credential problem. Nothing is lost — the code is complete and tested at 533 passing.

What has not been verified

#6 has had no live end-to-end run. It is unit-tested and CI-green, but the fixes were verified end-to-end on the reimbursement branch, not this one. One timesheet through the test repo after merging would close that — which needs the repoint above done first.

The Triage question needs a real Triage account. I am admin on these repos, so my own behaviour proves nothing about what a Triage collaborator can do. Two specifics matter: whether Triage can attach files to an issue, and whether a Triage author can edit the body of their own locked issue.

No SMTP failure drill was run. It was proposed and then dropped as unnecessary: the "email not sent" rendering is already unit-tested, and the processed-withheld behaviour was demonstrated for real by an unplanned failure (PR #40 has no processed; PR #42 does).