Repository navigation
refactor(birdhouse): stop routing errors through prose - #76
Open
gabrielsayers1 wants to merge 3 commits into
Open
gabrielsayers1 wants to merge 3 commits into
gabrielsayers1 wants to merge 3 commits into
Conversation
Editing a manifest or a skill takes effect on the next maintenance tick; editing src/ does not, because the worker loaded that code at start. A test run was lost to exactly this — the prompt fix was on disk while the worker still served the old copy, which reads as "the fix did not work" rather than "the fix is not running". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The HTTP layer chose status codes by regex-matching the English of errors thrown three files away: /run matched /unknown workflow|is disabled/, /claim matched each separately to split 404 from 400. Nothing kept the three message templates and two regexes in sync, so rewording a message — even to fix a typo — would silently turn a classified 4xx into a 500 with no test to catch it. Both entry points now share one lookup that raises a typed WorkflowLookupError carrying a machine-readable reason, and one onError handler maps reason to status. The routes lost their identical catch blocks and read as pure happy paths. That also fixes a contract split the reviewers found: /run answered 400 for both failures with the raw Error.message — internal function-name prefix and all — while /claim answered stable codes. They now agree. The insert paths stay separate on purpose: createWorkflowRun writes a pending row guarded by job idempotency, claimWorkflowRun writes a running row guarded by the partial unique index because the caller is already the agent. Only the validation prefix was accidental duplication. Also: /api/runs/:id/result had no try/catch at all, so a database blip on the busiest endpoint answered in plain text and broke the JSON contract the other routes promise; the shared handler covers it now. The dead "schedule" member of WorkflowRunTrigger is gone (claimWorkflowRun writes that row itself), the maintenance summary reports jobsPruned like its other two jobs, and three comments describing a deleted scheduler or an input_schema validator that never existed are corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two reviews of the previous commit, one adversarial and one on taste.
Deleting the routes/ own catch blocks quietly removed a blanket net: Hono
hands onError only what is instanceof Error and rethrows anything else
past it, where the node adapter answers an empty 500 with no body and
nothing logged. The logging middleware now normalises a non-Error throw
so everything reaches onError. The regression test claimed to cover this
and did not — it only ever threw an Error — so it now covers both shapes,
and I checked it fails without the fix rather than trusting that it would.
parseJsonBody's emptyBody parameter is gone. It existed because the result
callback fell back to undefined where the other two fall back to {}, but
that schema requires status, so an absent body is a 400 either way: the
knob never changed an outcome, and it cost a caveat comment longer than
the code it explained.
Also: both routes now say where their failures become responses, since a
reader starting at a route could not see it could fail at all; a leftover
changelist reference is out of a test; and the onError comment loses a
sentence the code beneath it already said.
cancelWorkflowRun keeps its plain Error on purpose. A typed reason exists
so a caller can branch without regex, and its only caller is the CLI,
which prints the message — the code now says so, and says what to do if
cancellation is ever exposed over HTTP.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
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
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.
Elegance pass over
infra/birdhouse, driven by four independent reviews: two over the package, then two over the resulting refactor (one adversarial, one on taste).The main change: stop routing errors through prose
createWorkflowRunandclaimWorkflowRuneach threwError("...unknown workflow..."), and the HTTP layer regex-matched that English to pick a status —/unknown workflow|is disabled/in/run, matched separately in/claimto split 404 from 400. Three independently-worded messages, two independently-written regexes, nothing keeping them in sync: rewording a message to fix a typo would have turned a classified 4xx into a 500, with no test to catch it.One shared lookup now raises a typed
WorkflowLookupErrorcarrying a machine-readable reason, and oneapp.onErrormaps reason to status. Both routes lost their identical catch blocks and read as happy paths.That also fixes a contract split:
/runanswered 400 for both failures with the rawError.message— internal function-name prefix and all — while/claimanswered stable codes. They now agree (404 unknown, 400 disabled), verified live.Deliberately not merged: the two insert paths.
createWorkflowRunwrites apendingrow guarded by job idempotency and hands off to a launch job;claimWorkflowRunwrites an already-runningrow guarded by the partial unique index, because the caller is the agent. Different lifecycles, different failure semantics — merging them would branch ontriggermid-insert and read worse. Only the validation prefix was accidental duplication./api/runs/:id/resulthad no error contractIt had no try/catch at all, so a database blip on the busiest route in the service fell through to Hono's default handler and answered plain text, breaking the JSON
{error}contract every other route promises. The shared handler covers it, with a regression test.Holes the first pass opened, found by review
onErroronly what isinstanceof Errorand rethrows anything else past it, where the node adapter answers an empty 500 with no body and nothing logged. The logging middleware now normalises non-Errorthrows. The regression test claimed to cover this and didn't — it only threwErrors — so it now covers both shapes, and I verified it fails without the fix rather than assuming it would.parseJsonBody'semptyBodyparameter is gone. It existed because the result callback fell back toundefinedwhere the others fall back to{}— but that schema requiresstatus, so an absent body is a 400 either way. The knob never changed an outcome and cost a caveat comment longer than the code it explained.Smaller
Dead
"schedule"member removed fromWorkflowRunTrigger(claimWorkflowRunwrites that row itself, so nothing could construct it);jobsPrunedreported in the maintenance summary like its other two jobs; comments corrected that described a deleted scheduler, aninput_schemavalidator that never existed, and a build-phasewave 2marker.Verification
pnpm typecheckclean; 121 tests pass underTZ=UTCandTZ=America/New_York. Both error contracts smoke-tested against the running worker on the box.Worth knowing
Every DB-touching suite here is gated on
BIRDHOUSE_TEST_DATABASE_URL, which CI does not set — so the tests in this PR, and all the existing ones, are invisible to the merge gate and rest on local runs. Fixing that means giving the CI test job a Postgres service; I've left it out of this PR since it touches shared CI, but it's the most valuable follow-up here.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.