fix(databases-on-aws): harden DSQL functional eval harness - #262
Conversation
6395330 to
8bf388c
Compare
anwesham-lab
left a comment
anwesham-lab
left a comment
There was a problem hiding this comment.
largely lgtm, leaving a comment for follow ups we should make, but they can be picked up after merged and I'd like to get the FK support in ASAP as priority
|
Reviewed this as the harness half of the #261 split. The split is clean and the blocking findings from #261 are genuinely fixed — no FK content leaked in (0 FK-related additions across the corpora diffs), so this reviews independently as intended. Recommending merge, with one small fix worth taking here and two follow-ups. Verified fixed, at head
The provenance block ( Smallest follow-up, could be in this PR: redaction corrupts UUIDsIn Only UUIDs with a hex letter in the final group redact cleanly — including the repo's own all-zeros example convention, which comes out as garbage. Since the DSQL evals are built on example UUIDs, artifacts and judge evidence carry mangled placeholders. Scope check before anyone worries: this is an evidence-quality bug, not a grading bug. I confirmed DDL structure survives redaction intact — column types and constraint clauses pass through — so no eval passes or fails wrongly because of it. Fix is to run Two things I suspected and disproved by testing, so they need no action: Follow-up 1: decompose the test suite
To be clear about severity, since "4 tests" reads worse than it is: I measured coverage, and those 4 tests exercise 83% of 2,832 statements in the runner. The fixes above are genuinely protected against regression. The cost is diagnosability — a failure reports one of four ~800-line tests like Follow-up 2:
|
8bf388c to
0d501ce
Compare
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Reviewed with a focus on path traversal and OS/network-level risk in the new harness. Two things I could not fault: no corpus field reaches a filesystem path (eval_id is type-checked as a non-negative int at 3801-3808 and is the only corpus value used as a path component), and no corpus content reaches argv (prompt and judge payload both go over stdin at 1187/1721, shell= is never used). The promotion path's O_NOFOLLOW + dir_fd + flock + inode identity discipline looks right.
Four blocking findings inline. Each performance and redaction claim was measured by importing the module and calling the real function on this branch, not inferred from pattern shape; timings are in the comments.
Please double-check these before I re-review — I would value a second opinion on two points specifically:
- The
_create_table_bodiestimings (comment on line 3552) are from adversarial input ("create table t("repeated). Do you consider a 2 MB unbalanced-DDL answer reachable in practice for aregexgrader eval, or is the realistic ceiling much lower than the constant permits? - Whether
signature/siggenuinely belong in_is_sensitive_key(line 2033) given_redact_tool_result_value's allowlist already covers the tool-result path. My concern is the artifact and judge paths, which are denylist-only — worth confirming that reading is right.
Non-blocking notes I did not comment on, happy to file separately if useful: env secrets under 12 chars are skipped inside mapping keys (2445); a bare 40-char AWS secret access key has no free-text detector (AWS_ENV_CREDENTIAL at 1928 requires a NAME= prefix); --model/--judge-model accept a leading - and become CLI flags; os.killpg at 659/697 runs after the child is already reaped at 951-957, so it can signal a recycled PID, and signal_known/wait_known drop the (pid, start_time) identity the Linux tracker already collects at 542-549; expanduser() at 1246 raises RuntimeError (not OSError) on an unknown ~user prefix, which aborts the whole run.
The known-limitation note about macOS double-fork plus setsid() is the right call to document rather than paper over.
There was a problem hiding this comment.
Suggested edits for three of the five blocking findings, each applied locally against 0d501ce and verified before posting. The harness suite passes with all three in place (python3 -m pytest -q test_run_functional_evals.py, 4 passed).
Two of my blocking comments I am withdrawing as written, because your own test suite documents intent that my suggestions would have broken. I tried both and they failed:
-
Line 4053 (hoisting the
MAX_REDACTION_INPUTguard out of theLLM_JUDGEbranch) — this breakstest_grading_correlates_tools_redacts_evidence_and_fails_closed:2044, which feeds a2 * MAX_ARTIFACT_TEXTanswer with a forbiddenCREATE INDEXburied in the middle and requires the regex grader to fail it. Grading long transcripts deterministically is deliberate, so the guard is correctly judge-only. The quadratic scanners are still the real problem; the line 3552 suggestion below fixes them at the source instead, which is the better place anyway. -
Line 5358 (gating the sibling sweep on the ownership marker) — this breaks
test_main_covers_both_graders_artifacts_and_incomplete_runs:2689, which creates an unmarked.results.run-injectedsibling and requires cleanup to remove it. Marker-gating cannot work here: a staging dir abandoned mid-crash may never have been marked. I still think deleting unmarked directories in an unowned parent is a sharp edge, but the fix has to preserve crash cleanup — perhaps confining staging to a subdirectory of the leased output dir, or matching on a per-run random suffix the runner records. Your call on the shape; I withdraw my specific suggestion.
The other two blocking findings (os.killpg on a reaped PID, and expanduser raising RuntimeError) I have left as prose rather than suggestions, since both fixes touch control flow where you will have better context than I do.
|
I'll pick up blocking comments separately, to strengthen evals and run evals locally and add to report in a comment in review of FKs to simultaneously unblock that workstream. |
|
Implemented the requested review follow-ups in Resolved:
The two withdrawn suggestions remain unchanged: the complete semantic-redaction limit is still judge-only, and unmarked crash staging is not marker-gated. Verification:
Working on running complete automated fleets across various judges on security and other areas. |
|
Final DSQL authoring/security convergence review completed at The contributor branch was rebased onto newer
Verification on the reviewed SHA:
No unresolved finding at the review procedure's confidence threshold remains. The documented macOS rapid double-fork plus |
Make functional eval runs fail closed around subprocess containment, transact gating, deterministic grading, artifact redaction, and output promotion. Validate all supported corpora under the versioned schema and add focused regression coverage for the harness safety boundaries. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the [project license](https://github.com/awslabs/agent-plugins/blob/main/LICENSE).
Bound adversarial SQL and redaction scans, cover SigV4 signature names, and avoid signaling reaped or recycled process identities. Fail closed on unresolvable Read paths, preserve unrelated sibling paths, and keep output recovery bound to the leased directory inode. Add focused regressions for each boundary and reject unsafe f-string and combined-DDL grading false positives.
fd3af00 to
4476945
Compare
blocking changes addressed
Complete 20+ agent review-fleet reportReviewed head: The contributor branch was rebased after the review commits were pushed. The Fleet coverage
Validated findings and dispositions
Reviewer-specific accounting
Verification
Only lightweight PR-title, contributor-statement, and merge-status jobs remain Conclusion: the 20+ agent review fleet is complete, every validated |
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Re-reviewed the updated diff at 4476945. All five of my blocking findings and the six smaller notes are fixed, and I verified each one rather than taking the diff's word for it.
What I confirmed resolved, by measurement on this branch:
| finding | evidence |
|---|---|
quadratic _create_table_bodies |
43.51 s -> 0.031 s at 100 KB; 0.125 s at 500 KB; real DDL verdicts unchanged |
unbounded key into SENSITIVE_KEY |
8.14 s -> 0.022 s at 30 KB |
| SigV4 names not sensitive | X-Amz-Signature/sig/hmac now True, signal/design/assignment still False, free text and artifacts both redact |
| sibling sweep in an unowned parent | the parent sweep is gone entirely; staging moved inside the leased dir via DescriptorTemporaryDirectory, and a planted .results.run-user-notes now survives |
killpg on a reaped PID, stale PID in signal_known |
returncode is None guard added; signal_known re-checks (pid, start_time) against /proc |
expanduser RuntimeError, --model leading dash, --pass-env startup controls, timeout bound |
all handled (_model_argument, MAX_TIMEOUT_SECONDS, UNSAFE_PASSTHROUGH_ENVIRONMENT) |
I also checked the new guard tests actually fail when the invariant breaks, rather than passing vacuously. Removing the scan budget, stripping SigV4 coverage from all five places, dropping the returncode guard, and reverting the RuntimeError handler each produced the matching failure. test_output_setup_preserves_unowned_sibling_runs is a nice reversal of the test that blocked my earlier suggestion — that resolved the tension cleanly, and moving staging under the lease is a better answer than the marker-gating I proposed.
That said, the revision is 1,174 changed runner lines, and reviewing the genuinely new code turned up four regressions I would block on, plus one hardening note. Three carry suggestions I applied and measured locally; the suite stays green (python3 -m pytest -q test_run_functional_evals.py, 51 passed) with all of them in place.
The one I would look at first is line 3686: the new _has_unsafe_sql_interpolation reintroduces exactly the unbounded-quadratic-scan-over-model-text pattern that this revision just fixed in _create_table_bodies, and it is reachable from the shipped safe_query_evals.json. Since that makes two independent instances, a cheap structural guard may be worth more than fixing them one at a time: bound the deterministic grading input once at the top of grade_eval (a text[:MAX_REDACTION_INPUT] local for the scanners, keeping raw text where anti-truncation matters), or add a test that asserts every registered rule grades a 2 MB adversarial answer within a few seconds.
CI is green on all substantive scans at this head (bandit, semgrep, Semgrep OSS, gitleaks, grype, checkov, clamav, sonarqube, dependency-review, zizmor, build); the four remaining checks are queued housekeeping.
Head branch was pushed to by a user without write access
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
All five follow-up findings are fixed at f71c3c3. Verified by running the real functions on this branch rather than reading the diff:
| finding | before | after |
|---|---|---|
_has_unsafe_sql_interpolation scan cost |
9.04 s at 78.6 KiB | 0.018 s at 78.6 KiB, 0.101 s at 449 KiB |
escaped separator (\:) leak |
secret passed through | <redacted-secret> |
| unterminated escaped assignment | 96 of 132 chars silently dropped | value redacted, trailing answer preserved |
TRUSTED_ARTIFACT_VALUE_KEYS subtree |
nested dict/list written verbatim | nested values redacted, scalar status still preserved |
OutputDirectoryLease.close() fd lifecycle |
injected unlock EIO left a stale fd that a second close() closed out from under an unrelated file |
close() survives the EIO, both fields land at -1, unrelated fd intact |
Detection is preserved on the regex rewrite: f-string, + concatenation, and % formatting all still return True, and a parameterised %s with cur.execute(sql, (...)) still returns False.
The twelve new guard tests are not vacuous. I reverted each of the five invariants in place and the matching test failed every time (test_unsafe_sql_interpolation_scan_is_bounded, test_escaped_sensitive_assignment_allows_escaped_separator, test_unterminated_escaped_assignment_preserves_trailing_answer, test_unterminated_escaped_password_redacts_value_and_preserves_tail, test_trusted_artifact_value_bypass_applies_only_to_scalars). Suite is 60 passed on a clean tree.
Correction to my severity ranking
My previous review led with the scan-cost finding and called it the thing to fix first. That ranking was wrong, and I want it on the record before this merges so the next reader does not inherit it.
I had assumed a CI or service threat model. This harness is neither. No workflow invokes run_functional_evals.py; CI only runs its unit tests via pytest tools plugins. Every documented invocation writes to /tmp, no raw artifact is committed anywhere in the repo, the corpus is in-repo under a CODEOWNERS rule covering *, and reaching a live cluster additionally requires the operator's own credentials plus an --mcp-config the README already labels as needing review. There is no multi-tenancy, no shared runner, and a human in front of every run.
Under that model:
- The transact guard is still the sharpest surface. Real credentials against a real cluster, write tool allowlisted, denial delegated to a hook. The
shlex.quotefix in the prior revision addressed my specific complaint, and this context confirms the concern rather than softening it. - The transcript truncation bug should have been ranked above the scan cost. The transcript is the product: a maintainer runs an eval, it fails, they read
transcript.json. Silently deleting the tail while grading used the full text means the artifact and the verdict disagree with no marker, in the one file a human will read. - The two redaction gaps hold at medium, but for a narrower reason than I gave. There is no live exfiltration path. What they defeat is the README's own control, which is a human reviewing artifacts before publication, and a plain
X-Amz-Signature=<64 hex>is what a skimming reader misses. - The fd lifecycle bug is local and single-process. Low.
- The scan cost is hygiene, not a blocker. Triggering it needs the subject model to emit several hundred kilobytes of adversarially shaped text in response to a reviewed prompt, and the cost is a maintainer waiting and pressing Ctrl-C. Worth bounding, which you did; not worth blocking on.
Bounding the deterministic grading input once in grade_eval is still worth doing as hygiene, since this was the second instance of the same unbounded-scan shape. test_unsafe_sql_interpolation_scan_is_bounded and test_create_table_body_scan_is_bounded do run in CI, so the class now has regression coverage either way.
Not approving yet only because CI for f71c3c3 is still queued. No outstanding code objections from me.
) * feat(databases-on-aws): add native DSQL foreign key support Part of the foreign key rollout with: - awslabs/aurora-dsql-orms#598 - awslabs/aurora-dsql-tools#147 * fix(databases-on-aws): align native DSQL foreign key guidance Present foreign keys as normal native DSQL functionality and remove retired application-layer replacement framing. Correct tenant-scoped optional relationship semantics, DSQL post-creation validation, OCC and SQLSTATE boundaries, deferral transaction scope, referential-action limits, and shared-parent modeling. Make table recreation relationship-safe with a schema-aware pre-create FK and dependent-view gate, single handling for self-references, exact restoration, a write fence, explicit destructive confirmation, and phase-specific recovery. Replace obsolete UNIQUE table recreation with documented async-index promotion, protect referenced keys during primary-key and AUTO_INCREMENT migrations, and correct focused MySQL, ORM, lint, and routing guidance. Expand the functional corpus to 19 prompts and 80 assertions covering dependency preflight, self-FKs, direct UNIQUE promotion, referenced-primary-key preservation, tenant nullability, and recovery. * fix(databases-on-aws): use direct constraint alterations Document direct CHECK, UNIQUE, constraint, default, and DROP NOT NULL operations while keeping table recreation only for true structural changes.\n\nSimplify generic table recreation to a dependency guard and user-approved bespoke plan, retain SELECT FOR UPDATE for write-skew decisions with OCC retry, and use concise foreign-key-constraint terminology.\n\nRebase FK eval semantics onto the schema-v2 harness from awslabs#262, remove the obsolete application-layer FK eval, use semantic grading for lint preservation assertions, and retain separate DSQL Lint follow-up work. --------- Co-authored-by: Anwesha Mukherjee <[email protected]>
4 participants
Summary
This is split from #261 so the eval harness hardening can be reviewed independently from the foreign key skill content.
Testing
mise run test:python(125 passed)mise run fmt:checkmise run build(lint, formatting, tests, Bandit, Semgrep, Gitleaks, Checkov, and Grype)git diff --checkReview follow-ups resolved
transact.sql_listpayload with lint source/fixed SQL and rejected comment-based bypasses%, and+SQL interpolationKnown limitation
macOS does not expose a supported primitive that reliably tracks a rapid double-fork plus
setsid()escape. The runner improveskqueue/libprocerror handling and cleanup, but does not claim that unsupported case is fully contained.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.