🧪 test(conformance): published tri-tool conformance matrix (#150) - #197
Merged
Conversation
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
August 5, 2026 18:34
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded a Docker-backed tri-tool conformance harness for current-standard, current-edge, and legacy-floor rows. The harness runs behavioral assertions, checks route drift, records metadata, and writes per-row JSON artifacts. Added Compose probe configuration, full-image overrides, self-tests, CI scheduling and aggregation, and pre-GA release checks. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
examples/compose/tri-tool/docker-compose.conformance-overlay.yml (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the probe image by digest.
The harness pins busybox by digest (
run-matrix.shlines 50-51) and pins every action by SHA. A floatingcurlimages/curl:8.12.1tag can be re-pushed and change harness behavior without a repo change. Add@sha256:....🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/compose/tri-tool/docker-compose.conformance-overlay.yml` at line 22, Update the probe image reference in the Docker Compose configuration to pin curlimages/curl:8.12.1 by its immutable sha256 digest, preserving the existing curl image version while replacing the floating tag reference.scripts/tri-tool-conformance/lib.sh (1)
99-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThree polling loops duplicate
wait_until.
wait_for_access_log_route,wait_for_log_line, andwait_for_container_log_lineeach re-implement the samewaited/sleep 2loop thatwait_untilalready provides, and each hardcodes a 2-second interval thatwait_untiltakes as a parameter. Express the three as predicates pluswait_untilcalls to keep one timing implementation. Note also thatwait_for_container_log_linematches case-insensitively (-Eiq) whilewait_for_log_linedoes not (-Eq); make that difference deliberate or remove it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tri-tool-conformance/lib.sh` around lines 99 - 147, Refactor wait_for_access_log_route, wait_for_log_line, and wait_for_container_log_line to define their existing checks as predicates and delegate polling, timeout, and interval handling to wait_until, using the intended 2-second interval. Preserve each function’s current success condition, and explicitly retain or remove the case-insensitive grep behavior in wait_for_container_log_line rather than changing it accidentally.scripts/tri-tool-conformance/run-matrix.sh (1)
559-564: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo identical requests to read a status and a body; the exec case has a side effect.
Lines 559-564 send the privileged exec create twice, and lines 658-659 send
POST /buildtwice. One request can return both. For the exec case the duplication is not free: if sockguard ever allows the request, the harness creates two exec instances and the FAIL message hides that it made two attempts.🔧 Proposed fix
- local denied_status denied_body - denied_status="$(probe_curl_status -X POST -H 'Content-Type: application/json' \ - -d '{"Cmd":["id"],"Privileged":true,"AttachStdout":true}' \ - "http://localhost/containers/${PRIMARY_SENTINEL_ID}/exec")" - denied_body="$(probe_curl -X POST -H 'Content-Type: application/json' \ - -d '{"Cmd":["id"],"Privileged":true,"AttachStdout":true}' \ - "http://localhost/containers/${PRIMARY_SENTINEL_ID}/exec")" + local denied_raw denied_status denied_body + denied_raw="$(probe_curl --write-out '\n%{http_code}' -X POST -H 'Content-Type: application/json' \ + -d '{"Cmd":["id"],"Privileged":true,"AttachStdout":true}' \ + "http://localhost/containers/${PRIMARY_SENTINEL_ID}/exec")" + denied_status="$(tail -n1 <<<"$denied_raw")" + denied_body="$(sed '$d' <<<"$denied_raw")"Consider adding a
probe_curl_status_and_bodyhelper inlib.shand using it at both sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 559 - 564, Replace the duplicated status/body requests in the privileged exec case and the POST /build case with a single probe_curl_status_and_body call. Add the helper in lib.sh to execute each request once while returning both HTTP status and response body, then update the run-matrix.sh assignments and parsing to use its outputs without changing the existing assertions or failure reporting..github/workflows/quality-tri-tool-conformance.yml (1)
132-139: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet
if-no-files-found: error.The comment on lines 128-131 states the artifact is written unconditionally, pass or fail. A missing file therefore means the row broke before
write_artifact, which is exactly the case the summary job cannot detect.warnhides it in the step log.🔧 Proposed fix
retention-days: 90 - if-no-files-found: warn + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/quality-tri-tool-conformance.yml around lines 132 - 139, Update the “Upload conformance artifact” step to set if-no-files-found to error instead of warn. Keep the existing artifact name, path, retention, and always condition unchanged so missing conformance files fail the upload step and remain detectable by the summary job.scripts/tri-tool-conformance/README.md (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language to the fenced command block.
markdownlint-cli2reports MD040 at Line 29. Change the opening fence tobash.Proposed fix
-``` +```bash🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tri-tool-conformance/README.md` around lines 29 - 34, Update the fenced command block in the README to use the bash language identifier on its opening fence, preserving all command examples unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/quality-tri-tool-conformance.yml:
- Around line 188-197: The conformance workflow must fail when any matrix row
produces no artifact. At .github/workflows/quality-tri-tool-conformance.yml
lines 188-197, move the artifact guards before the aggregation loop near line
170, retain the empty-artifact check, and assert that the count of
artifacts/*.json matches the matrix-row count before evaluating fail_count. At
lines 132-139, change the upload step’s if-no-files-found setting from warn to
error.
- Around line 33-36: Change the sockguard_image default in the workflow_dispatch
inputs to an empty value so manual dispatches without an explicit image use the
same compose-configured audited pin as scheduled runs. Keep the input optional
and require releasers to provide candidate image references explicitly.
In `@examples/compose/tri-tool/docker-compose.conformance-overlay.yml`:
- Around line 21-30: Update the probe service configuration to run as user and
group 65532:65532, preserving its existing image, entrypoint, security settings,
and socket volume mount.
In `@examples/compose/tri-tool/docker-compose.edge-exec.yml`:
- Around line 49-56: Validate the workflow_dispatch sockguard_image input in
run-matrix.sh before exporting SOCKGUARD_IMAGE or applying the
docker-compose.edge-exec.yml override. Accept only the approved trusted
registry/repository and require an immutable digest-pinned image reference;
reject all other values and stop execution before the sockguard service is
started.
In `@scripts/tri-tool-conformance/lib.sh`:
- Around line 103-108: Update the jq filter in wait_for_access_log_route to
tolerate missing or null normalized_path values by guarding the field before
applying test($p), and ensure valid non-object JSON lines cannot error when
accessing .msg by adding an object-type guard. Preserve matching behavior for
request, request_denied, and request_would_deny records with valid normalized
paths.
- Around line 58-61: Update the comment above the compose function to document
that compose executes the docker compose invocation directly, rather than
echoing a command for substitution. Replace the compose_cmd references and
$(compose_cmd) example with the actual compose API and direct subcommand usage.
In `@scripts/tri-tool-conformance/normalize-routes.jq`:
- Around line 10-15: Update assert_route_drift in run-matrix.sh to fail when
normalize-routes.jq produces no parseable access-log records, instead of
treating an empty observed route set as PASS. Preserve normalize-routes.jq’s
behavior of skipping malformed lines and non-access msg values, while ensuring
the empty observed result is reported as a parsing failure rather than route
parity.
In `@scripts/tri-tool-conformance/run-matrix.sh`:
- Around line 715-719: Update resolve_metadata in run-matrix.sh to remove the
compose pull before inspecting image digests, so metadata reflects the images
already used by the conformance run rather than newly resolved floating tags.
Preserve the existing digest inspection and unknown fallbacks; alternatively,
capture those digests immediately after assert_pristine_boot succeeds and reuse
them when generating the row metadata.
- Around line 464-475: In assert_logs, remove the pre-emptive record_result
calls for the lifecycle check when primary sentinel creation fails. Let the
existing assert_lifecycle handling of an empty PRIMARY_SENTINEL_ID produce the
single lifecycle result, avoiding duplicate summary entries and incorrect skip
classification.
- Around line 252-253: Explicitly check the `sudo chown` and `sudo chmod`
operations for both `portwing_ed25519.pem` and `portwing_authorized_keys` in the
matrix setup, rather than relying on the unchecked `&&` chains. Make each
failure terminate the row with a clear error so ownership or permission setup
failures are reported immediately.
- Around line 776-783: Update the abort handling around
assert_pristine_boot/assert_auth_handshake to record every assertion using the
exact result names emitted on the success path: include exec-policy-allowed,
exec-policy-denied, expected-denials-build, expected-denials-exec, and
expected-denials-export instead of aggregate names, and record auth-handshake as
skipped only when the handshake was not executed. Keep later assertion names
skipped for aborted rows.
- Around line 237-239: Update the standard-mode branch in run-matrix.sh to chown
the generated portwing_token.txt file to UID/GID 65532:65532 before applying
chmod 0400, ensuring the portwing service can read the mounted secret.
- Around line 43-51: Pre-pull both OLD_BUSYBOX_REF and NEW_BUSYBOX_REF on the
host before the assert_pristine_boot/create_sentinel flow in run-matrix.sh,
ensuring the pinned images are available through the sockguard proxy. Update the
nearby busybox pin comment to accurately describe the references if only one
matches busyboxPinnedRef.
---
Nitpick comments:
In @.github/workflows/quality-tri-tool-conformance.yml:
- Around line 132-139: Update the “Upload conformance artifact” step to set
if-no-files-found to error instead of warn. Keep the existing artifact name,
path, retention, and always condition unchanged so missing conformance files
fail the upload step and remain detectable by the summary job.
In `@examples/compose/tri-tool/docker-compose.conformance-overlay.yml`:
- Line 22: Update the probe image reference in the Docker Compose configuration
to pin curlimages/curl:8.12.1 by its immutable sha256 digest, preserving the
existing curl image version while replacing the floating tag reference.
In `@scripts/tri-tool-conformance/lib.sh`:
- Around line 99-147: Refactor wait_for_access_log_route, wait_for_log_line, and
wait_for_container_log_line to define their existing checks as predicates and
delegate polling, timeout, and interval handling to wait_until, using the
intended 2-second interval. Preserve each function’s current success condition,
and explicitly retain or remove the case-insensitive grep behavior in
wait_for_container_log_line rather than changing it accidentally.
In `@scripts/tri-tool-conformance/README.md`:
- Around line 29-34: Update the fenced command block in the README to use the
bash language identifier on its opening fence, preserving all command examples
unchanged.
In `@scripts/tri-tool-conformance/run-matrix.sh`:
- Around line 559-564: Replace the duplicated status/body requests in the
privileged exec case and the POST /build case with a single
probe_curl_status_and_body call. Add the helper in lib.sh to execute each
request once while returning both HTTP status and response body, then update the
run-matrix.sh assignments and parsing to use its outputs without changing the
existing assertions or failure reporting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 885d3df0-74d1-402e-8f63-00154a65e9bf
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (14)
.github/workflows/quality-tri-tool-conformance.yml.gitignoreRELEASING.mdexamples/compose/tri-tool/README.mdexamples/compose/tri-tool/docker-compose.conformance-overlay.ymlexamples/compose/tri-tool/docker-compose.edge-exec.ymlexamples/compose/tri-tool/docker-compose.ymlscripts/tri-tool-conformance-run-matrix.test.mjsscripts/tri-tool-conformance/README.mdscripts/tri-tool-conformance/known-routes.jsonscripts/tri-tool-conformance/lib.shscripts/tri-tool-conformance/normalize-routes.jqscripts/tri-tool-conformance/run-matrix.shscripts/tri-tool-conformance/testdata/access-log-fixture.jsonl
Publishes #150's Sockguard + Portwing + drydock conformance matrix. New weekly/workflow_dispatch quality-tri-tool-conformance.yml boots the audited examples/compose/tri-tool bundle from published images only (never source-built) across three rows -- current-standard, current-edge, and legacy-floor (the audited 0.8.1/1.5.2 pins from PR #155) -- and runs scripts/tri-tool-conformance/run-matrix.sh's ten ordered assertions per row: pristine fresh-volume boot, auth handshake plus one negative probe per mode, inventory/inspect, events, logs, lifecycle, configured exec (Edge only), the remote-update trigger, expected denials, and a route-drift tripwire that diffs sockguard's own access log against a checked-in known-routes.json manifest. - 🧪 scripts/tri-tool-conformance/{run-matrix.sh,lib.sh}: the driver + shared helpers; --self-test exercises the route normalizer and tripwire diff logic against a fixture with no Docker required - 🧪 scripts/tri-tool-conformance/normalize-routes.jq + known-routes.json: route-shape normalizer and the manifest seeded from app/configs/portwing.yaml / portwing-with-exec.yaml's allow rules - 🧪 scripts/tri-tool-conformance-run-matrix.test.mjs: wires --self-test into `npm test` - 🔧 examples/compose/tri-tool/docker-compose.conformance-overlay.yml: test-only probe container overlay; the audited bundle itself is untouched - 🔧 examples/compose/tri-tool/{docker-compose.yml,docker-compose.edge-exec.yml,README.md}: rename SOCKGUARD_VERSION (tag-only) to SOCKGUARD_IMAGE (full ref) so the harness and the new pre-GA release gate can target a release-candidate image on any of sockguard's three registries - 📝 CHANGELOG.md: Tests + Docs entries under [Unreleased]
Adds RELEASING.md step 4: before promoting a minor/major release candidate to GA, run the new quality-tri-tool-conformance.yml workflow against the candidate image (sockguard_image dispatch input) and require all three matrix rows green.
… short-circuit - sockguard_image dispatch input now defaults to empty (falls through to the compose bundle's own pinned default, same as a scheduled run) instead of ghcr.io/codeswhat/sockguard:latest, which pointed at a different registry/tag than the compose default a scheduled run uses. Input description and RELEASING.md's pre-GA gate step now say to pass the candidate ref explicitly. - summary job now asserts artifact count == matrix row count BEFORE aggregating, not just checking for total emptiness after -- a row that dies before write_artifact runs no longer passes the gate silently.
…wner curlimages/curl's own image default UID (100) can't reach a socket owned 65532:65532 mode 0600, so the probe's curl calls were failing on their own filesystem permission mismatch instead of exercising the socket mode the assertion actually cares about.
--sockguard-image gets interpolated into a compose file and pulled/run. The workflow that drives this script is workflow_dispatch/schedule only (no pull_request trigger, so the input is maintainer-controlled, not attacker-controlled via a PR), but validating it against the allowlisted sockguard registries/repos is defense in depth worth having regardless.
…on zero routes - wait_for_access_log_route's jq filter (lib.sh) now skips non-object JSON and missing/null normalized_path instead of erroring on them. Under `set -o pipefail`, a jq error on any line -- even one arriving after a real match was already printed -- flipped the whole pipeline's exit status non-zero and reported the wait as failed regardless of what grep actually found, poisoning assertions 3 (inventory-inspect) and 4 (events). - normalize-routes.jq gets the same select(type=="object") guard: a bare-string JSON line would otherwise abort the whole `[inputs | ...]` collection rather than just being skipped, poisoning assertion 10. - assert_route_drift now fails closed (route_drift_status, moved to lib.sh so --self-test can exercise it directly) when zero access-log records were captured, instead of recording a vacuous empty-diff PASS -- that almost always means log capture or the normalizer broke, not that nothing happened. - Extended testdata/access-log-fixture.jsonl with a missing-normalized_path line and a bare-string JSON line, and added self-test coverage for both the jq tolerance and the fail-closed behavior. - Fixed lib.sh's stale compose_cmd doc comment (the function is `compose` and runs directly, it doesn't echo an invocation for callers to expand).
…ts, aligned skip names - Pre-pull OLD_BUSYBOX_REF and NEW_BUSYBOX_REF explicitly before any assertion needs them, checked, instead of letting the first containers/create implicitly trigger the pull -- a slow/flaky pull on a shared runner otherwise reads as an unrelated timeout in whichever assertion needed the image first. Corrected the header comment: only NEW_BUSYBOX_REF matches app/integration/helpers_test.go's busyboxPinnedRef, and that repo's own pre-pull is for its own CI job, not this one. - portwing_token.txt (standard mode) is now chown'd to 65532:65532 before the chmod 0400 -- it stayed owned by the script runner while portwing reads it as UID 65532. Every chown/chmod in the secrets-setup block (standard and edge) is now checked explicitly instead of `&&`-chained silently, failing the row with a clear setup-error message instead of surfacing later as a confusing auth-handshake failure. - Image digests are now captured via `docker image inspect` right after `compose up` in assert_pristine_boot, from the images the row actually ran. resolve_metadata no longer does a `docker compose pull` at the end of the row, which could catch an upstream tag having moved mid-run and record a digest that never ran any of this row's assertions. - assert_logs no longer pre-emptively records a "lifecycle" FAIL when the primary sentinel create fails -- assert_lifecycle already records its own FAIL for that case, so the artifact was getting a duplicate entry. - The abort-path skip loop (pristine-boot/auth-handshake failure) now emits exactly the same assertion names the success path does (exec-policy-allowed/-denied instead of exec-policy on exec rows, expected-denials-build/-exec/-export instead of expected-denials), and adds a skipped "auth-handshake" entry for the case where pristine boot itself is what failed and auth-handshake was never even attempted.
scttbnsn
force-pushed
the
feat/150-tri-tool-conformance
branch
from
August 5, 2026 20:34
07f272c to
a8a837d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
scripts/tri-tool-conformance/run-matrix.sh (1)
90-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hardcoded
/tmppaths withmktemp.About 20 sites write fixed
/tmp/...names (known-routes-lint.err,keygen.err,busybox-pull.log,compose-up.log,badsecret.err,badkey-run.err,trigger-response.json, and others). Two effects: a pre-created symlink at any of those paths redirects the write, and two rows sharing a runner overwrite each other's diagnostics. The file already usesmktempinassert_route_driftand the probes.♻️ Proposed approach
+# One scratch dir per run; removed by the EXIT trap. +TMPDIR_RUN="$(mktemp -d)"Then reference
"${TMPDIR_RUN}/compose-up.log"and so on, and addrm -rf "$TMPDIR_RUN"tocleanup. Forrun_self_test, create its ownmktemp -dso the Docker-free path stays self-contained.Also applies to: 355-356, 392-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 90 - 91, Replace all fixed /tmp diagnostic and response paths in the script with files under a per-run temporary directory, using a securely created TMPDIR_RUN and updating every reference consistently. Remove that directory in cleanup, and have run_self_test create and use its own mktemp -d directory so its Docker-free execution remains self-contained.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tri-tool-conformance/lib.sh`:
- Around line 156-159: Update all three log-matching pipelines in
scripts/tri-tool-conformance/lib.sh: the route check using
ACCESS_LOG_ROUTE_MATCH_JQ, the case-sensitive pattern check, and the
case-insensitive pattern check must replace early-exiting grep consumers with
consumers that read the complete jq/log stream while preserving each existing
match mode. Add a regression fixture containing a matching line followed by more
than one pipe buffer of additional output, and verify all three checks succeed
without SIGPIPE-related failures.
In `@scripts/tri-tool-conformance/README.md`:
- Around line 36-40: Update the prerequisites list in the README to include sudo
alongside bash, curl, jq, docker, docker compose, and openssl, reflecting the
passwordless sudo requirement used by run-matrix.sh.
- Around line 29-34: Add the bash language identifier to the fenced command
block in the README usage example, changing the opening fence to ```bash while
preserving all commands and comments unchanged.
In `@scripts/tri-tool-conformance/run-matrix.sh`:
- Around line 435-436: The probe secret files are unreadable by the container
users because mktemp creates mode 0600 files. In
scripts/tri-tool-conformance/run-matrix.sh lines 435-436, make wrong_secret_file
mode 0444 after writing the invalid secret; in lines 470-477, make key_file mode
0444 after successful key generation so both probe containers can read their
mounted throwaway credentials.
- Around line 611-614: Update both skip paths in
scripts/tri-tool-conformance/run-matrix.sh: at lines 611-614, change the
PRIMARY_SENTINEL_ID-empty result to SKIP and return 0; at lines 650-653, record
${name}-allowed and ${name}-denied as SKIP and return 0, matching the names
emitted by the exec success path and abort loop.
- Around line 453-462: Update the `wait_for_container_log_line` matcher in the
wrong-secret probe to use the same anchored 401-response alternation as the edge
probe near line 493, rather than the bare `401` string. Keep the existing
PASS/FAIL handling and cleanup unchanged.
- Around line 417-421: Update the socket ownership check around owner_mode to
use a digest-pinned STAT_HELPER_REF defined alongside the existing busybox
references, and include that reference in the pre-pull loop. Separate helper
image pull failures from the ownership/mode assertion so pull/setup failures are
recorded distinctly rather than as an empty owner/mode conformance failure.
- Around line 263-264: Update the DOCKER_SOCK_GID resolution near the export so
it validates that at least one stat command returned a non-empty group ID; if
resolution fails, exit immediately with an explicit Docker socket error,
matching the existing fail-fast behavior used by the busybox pre-pull path.
Preserve the successful value export for supported stat implementations.
- Around line 738-741: Update the image value in the trigger request within the
matrix flow to pass the full NEW_BUSYBOX_REF, including its digest, instead of
stripping the `@sha256` suffix with parameter expansion. Preserve the existing
sentinel and request structure so the recreated container uses the pulled,
audited image reference.
- Around line 102-111: Update the fixture-count comment above the conformance
assertion so it describes exactly 10 lines and 6 unique route shapes: include
the deliberately unknown denied route within the six real access-log lines,
rather than counting it as an additional line, and keep the remaining
malformed/partial-line counts accurate.
---
Nitpick comments:
In `@scripts/tri-tool-conformance/run-matrix.sh`:
- Around line 90-91: Replace all fixed /tmp diagnostic and response paths in the
script with files under a per-run temporary directory, using a securely created
TMPDIR_RUN and updating every reference consistently. Remove that directory in
cleanup, and have run_self_test create and use its own mktemp -d directory so
its Docker-free execution remains self-contained.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1476d369-5700-4f5c-83d9-fd56767aa165
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (14)
.github/workflows/quality-tri-tool-conformance.yml.gitignoreRELEASING.mdexamples/compose/tri-tool/README.mdexamples/compose/tri-tool/docker-compose.conformance-overlay.ymlexamples/compose/tri-tool/docker-compose.edge-exec.ymlexamples/compose/tri-tool/docker-compose.ymlscripts/tri-tool-conformance-run-matrix.test.mjsscripts/tri-tool-conformance/README.mdscripts/tri-tool-conformance/known-routes.jsonscripts/tri-tool-conformance/lib.shscripts/tri-tool-conformance/normalize-routes.jqscripts/tri-tool-conformance/run-matrix.shscripts/tri-tool-conformance/testdata/access-log-fixture.jsonl
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/compose/tri-tool/README.md
- .gitignore
- scripts/tri-tool-conformance/known-routes.json
- examples/compose/tri-tool/docker-compose.edge-exec.yml
- .github/workflows/quality-tri-tool-conformance.yml
- scripts/tri-tool-conformance-run-matrix.test.mjs
- examples/compose/tri-tool/docker-compose.yml
- examples/compose/tri-tool/docker-compose.conformance-overlay.yml
- RELEASING.md
- scripts/tri-tool-conformance/normalize-routes.jq
- scripts/tri-tool-conformance/testdata/access-log-fixture.jsonl
Comment on lines
+156
to
+159
| if sockguard_access_log | jq -R \ | ||
| --arg m "$method" --arg d "$decision" --arg p "$path_regex" \ | ||
| "$ACCESS_LOG_ROUTE_MATCH_JQ" \ | ||
| 2>/dev/null | grep -q .; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Consume the full log stream before returning a match.
grep -q exits after the first match. If jq, docker compose logs, or docker logs writes more data, it can receive SIGPIPE. With pipefail, the helper returns failure although the required line exists.
scripts/tri-tool-conformance/lib.sh#L156-L159: replacegrep -q .with a consumer that reads alljqoutput.scripts/tri-tool-conformance/lib.sh#L174-L175: replacegrep -Eq "$pattern"with a full-stream consumer.scripts/tri-tool-conformance/lib.sh#L191-L192: replacegrep -Eiq "$pattern"with a full-stream consumer.
Proposed fix
- 2>/dev/null | grep -q .; then
+ 2>/dev/null | grep . >/dev/null; then
...
- if compose logs --no-color --no-log-prefix "$service" 2>/dev/null | grep -Eq "$pattern"; then
+ if compose logs --no-color --no-log-prefix "$service" 2>/dev/null | grep -E "$pattern" >/dev/null; then
...
- if docker logs "$container" 2>&1 | grep -Eiq "$pattern"; then
+ if docker logs "$container" 2>&1 | grep -Ei "$pattern" >/dev/null; thenAdd a regression fixture with a matching line followed by more than one pipe buffer of output.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if sockguard_access_log | jq -R \ | |
| --arg m "$method" --arg d "$decision" --arg p "$path_regex" \ | |
| "$ACCESS_LOG_ROUTE_MATCH_JQ" \ | |
| 2>/dev/null | grep -q .; then | |
| if sockguard_access_log | jq -R \ | |
| --arg m "$method" --arg d "$decision" --arg p "$path_regex" \ | |
| "$ACCESS_LOG_ROUTE_MATCH_JQ" \ | |
| 2>/dev/null | grep . >/dev/null; then |
Suggested change
| if sockguard_access_log | jq -R \ | |
| --arg m "$method" --arg d "$decision" --arg p "$path_regex" \ | |
| "$ACCESS_LOG_ROUTE_MATCH_JQ" \ | |
| 2>/dev/null | grep -q .; then | |
| if compose logs --no-color --no-log-prefix "$service" 2>/dev/null | grep -E "$pattern" >/dev/null; then | |
| return 0 |
Suggested change
| if sockguard_access_log | jq -R \ | |
| --arg m "$method" --arg d "$decision" --arg p "$path_regex" \ | |
| "$ACCESS_LOG_ROUTE_MATCH_JQ" \ | |
| 2>/dev/null | grep -q .; then | |
| if docker logs "$container" 2>&1 | grep -Ei "$pattern" >/dev/null; then | |
| return 0 |
📍 Affects 1 file
scripts/tri-tool-conformance/lib.sh#L156-L159(this comment)scripts/tri-tool-conformance/lib.sh#L174-L175scripts/tri-tool-conformance/lib.sh#L191-L192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/lib.sh` around lines 156 - 159, Update all three
log-matching pipelines in scripts/tri-tool-conformance/lib.sh: the route check
using ACCESS_LOG_ROUTE_MATCH_JQ, the case-sensitive pattern check, and the
case-insensitive pattern check must replace early-exiting grep consumers with
consumers that read the complete jq/log stream while preserving each existing
match mode. Add a regression fixture containing a matching line followed by more
than one pipe buffer of additional output, and verify all three checks succeed
without SIGPIPE-related failures.
Comment on lines
+29
to
+34
| ``` | ||
| scripts/tri-tool-conformance/run-matrix.sh --row current-standard | ||
| scripts/tri-tool-conformance/run-matrix.sh --row current-edge \ | ||
| --portwing-version 0.9.2 --drydock-version 1.6.0 | ||
| scripts/tri-tool-conformance/run-matrix.sh --self-test # jq only, no Docker | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fence.
markdownlint flags MD040 here.
📝 Proposed fix
-```
+```bash
scripts/tri-tool-conformance/run-matrix.sh --row current-standard📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| scripts/tri-tool-conformance/run-matrix.sh --row current-standard | |
| scripts/tri-tool-conformance/run-matrix.sh --row current-edge \ | |
| --portwing-version 0.9.2 --drydock-version 1.6.0 | |
| scripts/tri-tool-conformance/run-matrix.sh --self-test # jq only, no Docker | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/README.md` around lines 29 - 34, Add the bash
language identifier to the fenced command block in the README usage example,
changing the opening fence to ```bash while preserving all commands and comments
unchanged.
Source: Linters/SAST tools
Comment on lines
+36
to
+40
| Needs `bash`, `curl`, `jq`, `docker`, `docker compose`, and `openssl` on | ||
| `PATH`, plus a real Docker daemon at `/var/run/docker.sock` (the GitHub-hosted | ||
| `ubuntu-latest` runner ships all of this; there is no dind container, no | ||
| special privileges — the harness measures what a real deployment would | ||
| actually see, same rationale as `quality-integration.yml`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
List sudo as a prerequisite.
run-matrix.sh calls sudo chown/sudo chmod on the generated secrets and exits 1 if either fails (lines 302-340). A local run without passwordless sudo aborts before the first assertion. Add sudo to this list.
📝 Proposed fix
-Needs `bash`, `curl`, `jq`, `docker`, `docker compose`, and `openssl` on
-`PATH`, plus a real Docker daemon at `/var/run/docker.sock` (the GitHub-hosted
+Needs `bash`, `curl`, `jq`, `docker`, `docker compose`, `openssl`, and
+passwordless `sudo` (used to chown the generated secret/key material to the
+UIDs portwing and drydock run as) on
+`PATH`, plus a real Docker daemon at `/var/run/docker.sock` (the GitHub-hosted📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Needs `bash`, `curl`, `jq`, `docker`, `docker compose`, and `openssl` on | |
| `PATH`, plus a real Docker daemon at `/var/run/docker.sock` (the GitHub-hosted | |
| `ubuntu-latest` runner ships all of this; there is no dind container, no | |
| special privileges — the harness measures what a real deployment would | |
| actually see, same rationale as `quality-integration.yml`). | |
| Needs `bash`, `curl`, `jq`, `docker`, `docker compose`, `openssl`, and | |
| passwordless `sudo` (used to chown the generated secret/key material to the | |
| UIDs portwing and drydock run as) on | |
| `PATH`, plus a real Docker daemon at `/var/run/docker.sock` (the GitHub-hosted | |
| `ubuntu-latest` runner ships all of this; there is no dind container, no | |
| special privileges — the harness measures what a real deployment would | |
| actually see, same rationale as `quality-integration.yml`). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/README.md` around lines 36 - 40, Update the
prerequisites list in the README to include sudo alongside bash, curl, jq,
docker, docker compose, and openssl, reflecting the passwordless sudo
requirement used by run-matrix.sh.
Comment on lines
+102
to
+111
| # The fixture (testdata/access-log-fixture.jsonl) has 10 lines: 6 real | ||
| # access-log lines (5 allowed + 1 denied that IS in known-routes.json), | ||
| # 1 deliberately-unknown denied route (GET /containers/*/attach, which is | ||
| # NOT in known-routes.json -- attach is never allowed by any preset and | ||
| # was never added as an expected-denial-probe shape either), 1 | ||
| # access-log-shaped line with normalized_path missing entirely, 1 | ||
| # non-access-log line (msg=startup), 1 bare-string JSON value (valid JSON, | ||
| # not an object), and 1 line that isn't JSON at all. The three malformed/ | ||
| # partial lines prove tolerance (see below); a correct normalizer still | ||
| # yields exactly 6 unique {method,path} shapes from the 6 real lines. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fixture line count in this comment contradicts the assertion below.
The comment counts 6 real lines plus 1 deliberately-unknown route, which is 7 route-bearing lines and 11 lines total. The check at line 114 wants 6 shapes, and README lines 164-171 describe 6 real lines of which 1 is absent from the manifest. Fix the comment so the unknown route is one of the 6.
📝 Proposed fix
- # The fixture (testdata/access-log-fixture.jsonl) has 10 lines: 6 real
- # access-log lines (5 already in the manifest, 1 denied that IS in known-routes.json),
- # 1 deliberately-unknown denied route (GET /containers/*/attach, which is
- # NOT in known-routes.json -- attach is never allowed by any preset and
- # was never added as an expected-denial-probe shape either), 1
+ # The fixture (testdata/access-log-fixture.jsonl) has 10 lines: 6 real
+ # access-log lines -- 5 already in known-routes.json, plus 1
+ # deliberately-unknown denied route (GET /containers/*/attach, which is
+ # NOT in known-routes.json -- attach is never allowed by any preset and
+ # was never added as an expected-denial-probe shape either) -- 1
# access-log-shaped line with normalized_path missing entirely, 1
# non-access-log line (msg=startup), 1 bare-string JSON value (valid JSON,
# not an object), and 1 line that isn't JSON at all. The three malformed/
# partial lines prove tolerance (see below); a correct normalizer still
# yields exactly 6 unique {method,path} shapes from the 6 real lines.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # The fixture (testdata/access-log-fixture.jsonl) has 10 lines: 6 real | |
| # access-log lines (5 allowed + 1 denied that IS in known-routes.json), | |
| # 1 deliberately-unknown denied route (GET /containers/*/attach, which is | |
| # NOT in known-routes.json -- attach is never allowed by any preset and | |
| # was never added as an expected-denial-probe shape either), 1 | |
| # access-log-shaped line with normalized_path missing entirely, 1 | |
| # non-access-log line (msg=startup), 1 bare-string JSON value (valid JSON, | |
| # not an object), and 1 line that isn't JSON at all. The three malformed/ | |
| # partial lines prove tolerance (see below); a correct normalizer still | |
| # yields exactly 6 unique {method,path} shapes from the 6 real lines. | |
| # The fixture (testdata/access-log-fixture.jsonl) has 10 lines: 6 real | |
| # access-log lines -- 5 already in known-routes.json, plus 1 | |
| # deliberately-unknown denied route (GET /containers/*/attach, which is | |
| # NOT in known-routes.json -- attach is never allowed by any preset and | |
| # was never added as an expected-denial-probe shape either) -- 1 | |
| # access-log-shaped line with normalized_path missing entirely, 1 | |
| # non-access-log line (msg=startup), 1 bare-string JSON value (valid JSON, | |
| # not an object), and 1 line that isn't JSON at all. The three malformed/ | |
| # partial lines prove tolerance (see below); a correct normalizer still | |
| # yields exactly 6 unique {method,path} shapes from the 6 real lines. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 102 - 111, Update
the fixture-count comment above the conformance assertion so it describes
exactly 10 lines and 6 unique route shapes: include the deliberately unknown
denied route within the six real access-log lines, rather than counting it as an
additional line, and keep the remaining malformed/partial-line counts accurate.
Comment on lines
+263
to
+264
| export DOCKER_SOCK_GID | ||
| DOCKER_SOCK_GID="$(stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check DOCKER_SOCK_GID resolution.
If both stat forms fail, DOCKER_SOCK_GID is empty and gets interpolated into the compose files. compose up then fails inside assert_pristine_boot with a message that says nothing about the socket. Fail here instead, the same way the busybox pre-pull does.
🔧 Proposed fix
-export DOCKER_SOCK_GID
DOCKER_SOCK_GID="$(stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock)"
+if [ -z "$DOCKER_SOCK_GID" ]; then
+ echo "FATAL: could not read the GID of /var/run/docker.sock -- setup error, not a conformance failure" >&2
+ exit 1
+fi
+export DOCKER_SOCK_GID📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export DOCKER_SOCK_GID | |
| DOCKER_SOCK_GID="$(stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock)" | |
| DOCKER_SOCK_GID="$(stat -c '%g' /var/run/docker.sock 2>/dev/null || stat -f '%g' /var/run/docker.sock)" | |
| if [ -z "$DOCKER_SOCK_GID" ]; then | |
| echo "FATAL: could not read the GID of /var/run/docker.sock -- setup error, not a conformance failure" >&2 | |
| exit 1 | |
| fi | |
| export DOCKER_SOCK_GID |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 263 - 264, Update
the DOCKER_SOCK_GID resolution near the export so it validates that at least one
stat command returned a non-empty group ID; if resolution fails, exit
immediately with an explicit Docker socket error, matching the existing
fail-fast behavior used by the busybox pre-pull path. Preserve the successful
value export for supported stat implementations.
Comment on lines
+417
to
+421
| owner_mode="$(docker run --rm -v "${vol}:/v:ro" alpine stat -c '%u:%g %a' /v/sockguard.sock 2>/dev/null)" | ||
| if [ "$owner_mode" != "65532:65532 600" ]; then | ||
| record_result "$name" FAIL "socket owner/mode = '${owner_mode}', want '65532:65532 600' with no manual chown" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unpinned alpine turns a pull failure into a fake socket-ownership FAIL.
alpine is neither pinned nor pre-pulled, unlike the busybox refs (lines 53-54, 354-359). If the implicit pull fails, owner_mode is empty and the row records socket owner/mode = '', which reads as a conformance failure instead of a setup failure. Pin the ref, pre-pull it with the busybox loop, and separate the two failure modes.
🔧 Proposed fix
- owner_mode="$(docker run --rm -v "${vol}:/v:ro" alpine stat -c '%u:%g %a' /v/sockguard.sock 2>/dev/null)"
+ owner_mode="$(docker run --rm -v "${vol}:/v:ro" "$STAT_HELPER_REF" stat -c '%u:%g %a' /v/sockguard.sock 2>/tmp/stat-helper.err)"
+ if [ -z "$owner_mode" ]; then
+ record_result "$name" FAIL "could not stat the socket via ${STAT_HELPER_REF} -- setup error, not an ownership failure: $(cat /tmp/stat-helper.err)"
+ return 1
+ fi
if [ "$owner_mode" != "65532:65532 600" ]; thenDefine STAT_HELPER_REF next to the busybox pins as a digest-pinned ref, and add it to the pre-pull loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 417 - 421, Update
the socket ownership check around owner_mode to use a digest-pinned
STAT_HELPER_REF defined alongside the existing busybox references, and include
that reference in the pre-pull loop. Separate helper image pull failures from
the ownership/mode assertion so pull/setup failures are recorded distinctly
rather than as an empty owner/mode conformance failure.
Comment on lines
+435
to
+436
| wrong_secret_file="$(mktemp)" | ||
| openssl rand -hex 32 > "$wrong_secret_file" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Probe secret files are mode 0600 and owned by the runner, so the probe containers cannot read them. mktemp creates 0600 files owned by the script user, and both probe docker run invocations pass no --user, so each container reads the mount as its image UID and gets EACCES. Both probes then fail on a file-read error and record FAIL without ever exercising the credential check. Both files hold deliberately-invalid, throwaway material, so world-readable is acceptable.
scripts/tri-tool-conformance/run-matrix.sh#L435-L436: addchmod 0444 "$wrong_secret_file"after theopenssl randwrite, so drydock (UID 1000, see line 333) can read/run/secrets/portwing_token.scripts/tri-tool-conformance/run-matrix.sh#L470-L477: addchmod 0444 "$key_file"after the keygen write succeeds, so portwing (UID 65532, see line 325) can read/run/secrets/portwing_key.
📍 Affects 1 file
scripts/tri-tool-conformance/run-matrix.sh#L435-L436(this comment)scripts/tri-tool-conformance/run-matrix.sh#L470-L477
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 435 - 436, The probe
secret files are unreadable by the container users because mktemp creates mode
0600 files. In scripts/tri-tool-conformance/run-matrix.sh lines 435-436, make
wrong_secret_file mode 0444 after writing the invalid secret; in lines 470-477,
make key_file mode 0444 after successful key generation so both probe containers
can read their mounted throwaway credentials.
Comment on lines
+453
to
+462
| local ok=1 | ||
| wait_for_container_log_line "$bad_container" '401' 30 && ok=0 | ||
| docker rm -f "$bad_container" >/dev/null 2>&1 || true | ||
| rm -f "$wrong_secret_file" | ||
|
|
||
| if [ "$ok" -eq 0 ]; then | ||
| record_result "$name" PASS "throwaway agent config with the wrong shared secret observed a 401, matching the documented failure mode" | ||
| else | ||
| record_result "$name" FAIL "throwaway wrong-secret probe never logged a 401 within 30s" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
401 is too loose a log matcher.
The bare string 401 matches byte counts, ports, and timestamps, so the probe can PASS without an auth rejection. Use an alternation, as the edge probe does at line 493.
🔧 Proposed fix
- wait_for_container_log_line "$bad_container" '401' 30 && ok=0
+ wait_for_container_log_line "$bad_container" '\b401\b|[Uu]nauthorized' 30 && ok=0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local ok=1 | |
| wait_for_container_log_line "$bad_container" '401' 30 && ok=0 | |
| docker rm -f "$bad_container" >/dev/null 2>&1 || true | |
| rm -f "$wrong_secret_file" | |
| if [ "$ok" -eq 0 ]; then | |
| record_result "$name" PASS "throwaway agent config with the wrong shared secret observed a 401, matching the documented failure mode" | |
| else | |
| record_result "$name" FAIL "throwaway wrong-secret probe never logged a 401 within 30s" | |
| fi | |
| local ok=1 | |
| wait_for_container_log_line "$bad_container" '\b401\b|[Uu]nauthorized' 30 && ok=0 | |
| docker rm -f "$bad_container" >/dev/null 2>&1 || true | |
| rm -f "$wrong_secret_file" | |
| if [ "$ok" -eq 0 ]; then | |
| record_result "$name" PASS "throwaway agent config with the wrong shared secret observed a 401, matching the documented failure mode" | |
| else | |
| record_result "$name" FAIL "throwaway wrong-secret probe never logged a 401 within 30s" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 453 - 462, Update
the `wait_for_container_log_line` matcher in the wrong-secret probe to use the
same anchored 401-response alternation as the edge probe near line 493, rather
than the bare `401` string. Keep the existing PASS/FAIL handling and cleanup
unchanged.
Comment on lines
+611
to
+614
| if [ -z "$PRIMARY_SENTINEL_ID" ]; then | ||
| record_result "$name" FAIL "skipped -- primary sentinel was never created (see the logs assertion)" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Two skip paths record status FAIL instead of SKIP. When PRIMARY_SENTINEL_ID is empty, the logs assertion has already recorded the real failure. Recording FAIL again in the downstream assertions adds extra failures for one root cause and inflates the summary job's FAIL count. record_result supports SKIP, and the workflow counts SKIP separately.
scripts/tri-tool-conformance/run-matrix.sh#L611-L614: change torecord_result "$name" SKIP "primary sentinel was never created (see the logs assertion)"andreturn 0.scripts/tri-tool-conformance/run-matrix.sh#L650-L653: record${name}-allowedand${name}-deniedas SKIP andreturn 0, matching the names the exec success path and the abort loop at lines 914-916 emit.
📍 Affects 1 file
scripts/tri-tool-conformance/run-matrix.sh#L611-L614(this comment)scripts/tri-tool-conformance/run-matrix.sh#L650-L653
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 611 - 614, Update
both skip paths in scripts/tri-tool-conformance/run-matrix.sh: at lines 611-614,
change the PRIMARY_SENTINEL_ID-empty result to SKIP and return 0; at lines
650-653, record ${name}-allowed and ${name}-denied as SKIP and return 0,
matching the names emitted by the exec success path and abort loop.
Comment on lines
+738
to
+741
| trigger_status="$(curl --silent --show-error --max-time 30 --output /tmp/trigger-response.json --write-out '%{http_code}' \ | ||
| -X POST -H 'Content-Type: application/json' \ | ||
| -d "$(jq -n --arg c "$sentinel" --arg img "${NEW_BUSYBOX_REF%%@*}" '{container:$c, image:$img}')" \ | ||
| "http://127.0.0.1:3000/api/triggers/docker/update" 2>/dev/null)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does `docker pull name:tag@digest` create the local tag, per Docker docs?
curl -s https://docs.docker.com/reference/cli/docker/image/pull/ | sed -n 's/<[^>]*>//gp' | grep -in -A6 'digest' | head -60Repository: CodesWhat/sockguard
Length of output: 7630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context around trigger and pre-pull =="
sed -n '330,370p' scripts/tri-tool-conformance/run-matrix.sh
sed -n '720,750p' scripts/tri-tool-ccomformance/run-matrix.sh 2>/dev/null || sed -n '720,750p' scripts/tri-tool-conformance/run-matrix.sh
echo "== variable definition =="
rg -n "NEW_BUSYBOX_REF|docker pull|digest|update" scripts/tri-tool-conformance/run-matrix.shRepository: CodesWhat/sockguard
Length of output: 6590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file context around trigger and pre-pull =="
sed -n '330,370p' scripts/tri-tool-conformance/run-matrix.sh
sed -n '720,750p' scripts/tri-tool-conformance/run-matrix.sh
echo "== variable definition and relevant patterns =="
rg -n "NEW_BUSYBOX_REF|docker pull|digest|docker/create|/api/triggers/docker/update|assert_route_drift|resolve_metadata" scripts/tri-tool-conformance/run-matrix.sh
echo "== Docker pull tagged digest example in docs =="
curl -s https://docs.docker.com/reference/cli/docker/image/pull/ | sed -n 's/<[^>]*>//gp' | grep -i -C8 'tag.*digest\|digest.*tag\|ubuntu@sha256' | head -120Repository: CodesWhat/sockguard
Length of output: 12995
Forward the digest-pinned image ref to the update trigger.
docker pull "$NEW_BUSYBOX_REF" pulls the pinned busybox:1.37@sha256:..., but line 740 strips @sha256:... before sending image. The recreated container can resolve to the mutable busybox:1.37 tag instead of the audited digest. Send ${NEW_BUSYBOX_REF} or tag the pulled image before the trigger request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tri-tool-conformance/run-matrix.sh` around lines 738 - 741, Update
the image value in the trigger request within the matrix flow to pass the full
NEW_BUSYBOX_REF, including its digest, instead of stripping the `@sha256` suffix
with parameter expansion. Preserve the existing sentinel and request structure
so the recreated container uses the pulled, audited image reference.
This was referenced Aug 5, 2026
This was referenced Aug 6, 2026
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
1 participant
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.
Summary
Implements #150: a conformance harness that boots the audited
examples/compose/tri-tool/bundle from published images only and asserts the Sockguard ↔ Portwing ↔ drydock behavior contract those images' presets document.Matrix rows
current-standardlatestlatestcurrent-edgelatestlatestlegacy-floor0.8.11.5.2(+DD_EXPERIMENTAL_PORTWING=true)legacy-floor's versions are hardcoded (the audited floor from #155) and are not overridable by dispatch inputs — overriding it would defeat the point of a floor row.Assertions (10, in order)
65532:65532mode600, real_pingHandshake successfulin drydock logs, plus a negative probe per mode (wrong shared secret / unregistered Ed25519 key) via throwaway containers that never touch the row's real containersGET /containers/json+ inspect traffic/events+DELETEboth allowed in the access logGET /containers/{id}/logsthrough the proxydocker inspectconvergencecurrent-edgeonly: non-privileged exec allowed,Privileged: trueexec denied 403 with reasonlegacy-floorexpects501/build,/exec(non-exec preset),/containers/*/exportall denied(method, normalized_path)shape must be inknown-routes.json; anything new fails the job until policy is reviewed and the manifest updated in the same PRVerification strategy
Portwing's and drydock's own HTTP/WS API shapes aren't pinned anywhere in this repo. Rather than guess at them, assertions 3–9 drive Docker Engine API calls directly through sockguard's proxied socket (the exact shape Portwing sends, per the presets) and verify outcomes against sockguard's own structured access log — the one interface this repo fully owns and tests. This proves the sockguard-side half of every contract; it does not re-verify Portwing's/drydock's internal wiring.
Route-drift tripwire
scripts/tri-tool-conformance/normalize-routes.jqnormalizes each observed(method, normalized_path)into a shape (fixed Docker API keywords kept literal, dynamic segments →*, runs of dynamic segments →**), diffed againstknown-routes.json(seeded by hand-enumerating every allow rule inapp/configs/portwing.yaml+portwing-with-exec.yaml, plus the two deliberate-denial probe shapes). One-directional: an unused manifest entry isn't a failure, only an unexpected observed route is. This is the literal implementation of "a route added by either peer repository fails conformance until Sockguard policy is reviewed."What could NOT be verified in sandbox, and how the first dispatch confirms it
No live Docker daemon / published tri-tool stack was available here, so the following were verified as far as offline tooling allows and are flagged in
scripts/tri-tool-conformance/README.md's "Known gaps" section for confirmation on the first liveworkflow_dispatch(see the new RELEASING.md pre-GA gate step):POST /api/triggers/docker/updateon drydock's port 3000 is a best-effort match to the tri-tool README's compatibility prose (501from Portwing Standard mode today), not pinned by a test in either peer repo. May need a follow-up patch if the real contract differs.assert_standard_wrong_secret_probewatches for401in drydock's own logs rather than calling a specific portwing endpoint, but the exact log text is inferred from the README's troubleshooting section, not pinned.assert_edge_unknown_key_probematchesbad-signature|unknown-key|unauthorized|reject(case-insensitive), same inference caveat.None of these gaps touch assertions 1, 3–7, 9, or 10, which are grounded entirely in sockguard's own documented, tested surface.
What WAS verified in sandbox
shellcheckclean onrun-matrix.shandlib.shzizmorclean onquality-tri-tool-conformance.yml(both standalone and full-repo scan against the project's.github/zizmor.yml)docker compose config --quietvalidates for all 3 rows' exact env-var combinations against the bundle + newdocker-compose.conformance-overlay.ymlrun-matrix.sh --self-test— exercisesnormalize-routes.jq+ theknown-routes.jsondiff againsttestdata/access-log-fixture.jsonl, confirming exactly 6 shapes and isolating exactly the one deliberately-unknown route (GET /containers/*/attach)npm test(99/99 passing, including the newscripts/tri-tool-conformance-run-matrix.test.mjswrapper)npx biome check .clean (pre-existing unrelated warnings only)Non-goals (per design doc)
No new portwing/drydock preset unit tests were added to
app/integration/— filed as a follow-up issue instead (see below), since that's source-level testing rather than published-artifact conformance.Follow-up
Filed #196 —
test(integration): add portwing/drydock preset conformance tests, referencing this PR/#150.Test plan
workflow_dispatchrun of.github/workflows/quality-tri-tool-conformance.ymlon this branch (or after merge) — confirm all 3 matrix rows pass, resolve the three "Known gaps" above if they surface issuesself-testjob (jq-only, no Docker) — verified locally, passes in CI on pushdocker compose configvalidated for all 3 rowsshellcheck/zizmorcleannpm testpassingDo not merge until the first live dispatch run confirms the matrix passes — the sandbox this PR was built in has no Docker daemon or published tri-tool images to run against.
Changelog
current-standard,current-edge, andlegacy-floormatrix rows.SOCKGUARD_IMAGEreferences.no-new-privileges.Concerns