fix(conformance): body-check every accepted trigger refusal - #299
Merged
Conversation
CodeRabbit review on the #298 promotion. The 404 arm passed on the bare status, so a wrong trigger URL -- Express answers those with 404 "Cannot POST /..." -- would have passed assertion 8 while testing nothing. That's the same shape as the bug this series exists to close: the store poll fed a 410 into jq, swallowed the error, and produced a result indistinguishable from a real failure. Both accepted refusals are now identified by their response body: 404 "trigger not found", 400 "no update available". 501 leaves the accepting arm. It was accepted unvalidated and no row has ever returned it: the 2026-08-20 run recorded 404 on legacy-floor (drydock 1.5.2) and 400 on both current-* rows (1.6.x). The 501 in the bundle README is Portwing's own trigger endpoint, a different service from the drydock:3000 API this posts to. It now falls to the default arm, which prints the status and body, so a real 501 becomes a pinnable fact instead of a silent pass on a status nobody has observed.
The first cut of the body check asserted the literal "trigger not found", which the README has claimed since #211. Run 32323163685 shows that isn't what drydock says. legacy-floor failed with: {"error":"Remote update trigger portwing.docker.update not found"} The middle of that string is the agent-qualified trigger name, so it varies by row. Match `trigger .*not found` instead, which still rejects a bare {"error":"Not Found"} and Express's Cannot POST body from an unrouted request. Correct the wording in the README and the changelog. Also record drydock's actual error string in the 404 PASS detail. The previous artifacts carried only the harness's own summary, so after the fact there was no way to tell what drydock had actually answered -- which is exactly why this wrong assertion could be written in the first place.
npm dedupe collapses fumadocs-mdx's nested magic-string from 1.2.1 to 1.2.2, which is what the workspaces already resolve. Three lines: version, resolved URL, integrity hash. No package.json change and no behavioural surface. Surfaced by the pre-push lockfile-dedupe gate, which blocks a push while the lockfile and a dedupe disagree, so this rides on the branch that hit it rather than waiting for a PR of its own. Worth noting the install probe reported the tree as matching the lockfile: npm install --dry-run accepts a newer version that still satisfies the range, while dedupe reports it.
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
August 20, 2026 02:20
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
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. Comment |
biggest-littlest
approved these changes
Aug 20, 2026
biggest-littlest
left a comment
biggest-littlest
left a comment
Member
There was a problem hiding this comment.
Approved. All three conformance rows green on run 32323886778, and the artifacts now carry drydock's actual refusal string.
scttbnsn
added a commit
that referenced
this pull request
Aug 20, 2026
…es to main (#298) Promotion of dev/v1.7 into main. #294 / #299 -- the tri-tool conformance harness stops swallowing its own errors (closes #289). Assertion 8 polled drydock's unversioned /api/containers and /api/triggers/docker/update, which 410 as of drydock v1.6.0. The 410 body fed straight into jq, the type error went to 2>/dev/null, and what came back was an empty result indistinguishable from a real store-sync failure -- which is how a harness bug got reported as a drydock regression and sat for a month. Nothing was ever wrong with drydock. Both URLs move to /api/v1; the poll checks the HTTP status and the {data: [...]} envelope before trusting anything; and both accepted trigger refusals are now identified by their response body rather than a bare status, since a wrong URL also answers 404. 501 leaves the accepting arm: no row has ever returned it. #296 -- the lockfile-dedupe pre-push hook stops misnaming a stale node_modules as lockfile drift (closes #295). It probes npm install --dry-run first and names npm ci, and only then does the dedupe report get to mean anything. Logic moved into scripts/lockfile-dedupe.mjs with tests. Verified on conformance run 32323886778: all three rows and the harness self-test green, with drydock's actual refusal strings recorded in the artifacts. Fixes: #289 Fixes: #295
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
3 participants
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.
CodeRabbit's review on the #298 promotion, taken and then corrected by a real run.
The finding
Assertion 8's
404|501arm passed on the bare status. Express answers an unknown route with404 Cannot POST /..., so a wrong trigger URL would have passed the assertion while testing nothing. That is the same failure this whole series is about: the store poll fed a 410 intojq, swallowed the error, and produced a result indistinguishable from a real failure. A probe that cannot tell "I am broken" from "the thing I watch is refused as designed" is not a probe.501also left the accepting arm. It was accepted unvalidated and no row has ever returned it — the run artifacts record404onlegacy-floor(drydock 1.5.2) and400on bothcurrent-*rows (1.6.x). The501in the bundle README is Portwing's trigger endpoint, a different service from thedrydock:3000API this posts to. It now falls to the default arm, which prints the status and body, so a real501becomes a pinnable fact instead of a silent pass on a status nobody has observed.The correction
My first cut asserted the literal
"trigger not found", which the README has claimed since #211. Run 32323163685 failedlegacy-floorand showed why:The documented wording was wrong, and it had never been checked because nothing ever read the body. The middle of that string is the agent-qualified trigger name, so it varies by row. The match is
trigger .*not found, which still rejects a bare{"error":"Not Found"}and Express'sCannot POSTbody. README and CHANGELOG corrected.The
404PASS detail now records drydock's actualerrorstring. The previous artifacts carried only the harness's own summary, which is exactly why a wrong assertion could be written against them.Verification
Run 32323886778, all three rows plus the self-test green, and the artifacts now carry the evidence:
Note on the third commit
build(deps): dedupe the nested magic-string in the lockfileis unrelated to the above and is not something I went looking for. The pre-pushlockfile-dedupegate blocked the push with genuine drift infumadocs-mdx's nestedmagic-string(1.2.1 → 1.2.2), and unlike the false-positive class #296 just fixed,npm dedupehere produced a real three-line lockfile change. Kept as its own commit rather than folded in.Worth recording: the install probe reported the tree as matching the lockfile while the dedupe probe reported drift.
npm install --dry-runaccepts a newer version that still satisfies the range;dedupereports it. The two probes are not redundant.