Skip to content

test: objstore_allowlist gets its pytest twin, and its admin-override arm can now fail; seeds the ledger (#1341) - #1379

Merged
jdatcmd merged 21 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-objstore-allowlist-twin
Oct 15, 2026
Merged

jdatcmd merged 21 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-objstore-allowlist-twin

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Part of #1341, the object-store twin-parity item: objstore_allowlist.sh had no pytest twin. Writing the twin turned up a vacuous arm in the shell suite, which is fixed here in both twins.

The vacuous arm

"a superuser session SET works (admin override)" ran while the configured list was still other.example, 127.0.0.1. So the read succeeded whatever the session SET did, and the arm couldn't fail. Both twins now clear the configured list first, so only the session's SET can admit the read.

The setting defined PGC_SIGHUP (no session may SET it), PG17, main 4027b8d against this branch:

main this branch
shell 15 + 1 failed: the admin-override arm passed 14 + 2 failed: "a superuser session SET works (admin override): not a measurement… got []"
pytest twin none red at the first affected check, "a non-superuser cannot widen the list (42501): got '55P02'"

The twin (test_objstore_allowlist.py)

One test, all 16 shell checks by name, added to COMPLETE:

  • How the list is changed: the operator's way, ALTER SYSTEM and a reload, reset in a finally, because the cluster is shared.
  • The fixture server is the file's own: a Range-capable http.server in a thread, serving one Parquet file from server_dir. The shell's server is a script under test/, and the two harnesses don't call each other.
  • Its premise is a real HEAD to that server.
  • pyarrow: taken through expect.require_module.

Twin proof: an empty list that admits every endpoint reddens the two default-deny arms in the shell, on main and here, and the twin at "default: an http read is refused 42501: got ''".

Evidence

🤖 Generated with Claude Code

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The admin-override fix is right, and I verified it. One premise breaks #1236. Reviewed on PG18 at 323ffab.

The vacuous arm, and its fix. With objstore_allowed_endpoints changed from PGC_SUSET to PGC_SIGHUP (src/columnar_tableam.c:4329), main's shell fails only one check:
FAIL a non-superuser cannot widen the list (42501): got [55P02] want [42501]
The admin-override arm passed with the superuser's SET refused. On this PR it fails too:
FAIL a superuser session SET works (admin override): not a measurement, so nothing was compared: got [] want [1000]

The allow-list itself: with if (!ok) disabled (objstore/columnar_objstore_module.c:328), the shell fails 4. Among them:
FAIL default: an http read is refused 42501: got [] want [42501]
The twin also fails, and compare_to_bash shows 17 matches, 0 missing.

What to change:

  1. "premise: the list defaults to empty" goes red when the GUC default changes (src/columnar_tableam.c:4328, "" → "127.0.0.1"):
    FAIL premise: the list defaults to empty: got [127.0.0.1] want []
    Drop the prefix in both twins and date the row with that mutation. The name came from main, but this PR seeds it.
  2. Optional: the if (!ok) mutation also reddens "unlisted host" and "host:port entry, wrong port", which you seed as never. You could date them.
  3. Rebase: TESTS.md §105, expected_tests.txt and the budget conflict.

One environment note: /root/pyenv here has no pyarrow, so the twin declines and reports "1 passed" with 0 checks. That's correct behaviour, but a green run without pyarrow proves nothing. My pytest results above used the system pyarrow.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Both points in, plus your optional one: now 9f32f89, rebased on main.

  1. "the list defaults to empty" is a check now, in both twins, renamed in the ledger with its history. With the default changed to 127.0.0.1, both twins are red (got [set: [127.0.0.1]] want [empty]), and it's dated by it. The shell compared against an empty string, so a query that never ran also passed. Now the query itself produces empty, or set: […] otherwise.
  2. Optional, taken: with the allow-list check disabled (if (false && !ok)), "unlisted host" and "host:port entry, wrong port" are red too, and both are dated, alongside the two default arms.
  3. Rebased: TESTS.md §109, cluster_tests 549 → 550; the seed is restated, giving 2529 rows, never 1937, ceiling 242 → 241. The gate is clean, and the guard set is 433/433.

Runs: 16/16 and the twin passing (with PGC_REQUIRE=pyarrow) on each of 15–19. Main hasn't changed the suite since the base.

On your environment note: agreed. A twin that declines without pyarrow proves nothing, so all my runs set PGC_REQUIRE=pyarrow, which turns a decline into a failure.

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 14, 2026
…d the shell names 'empty' itself (commandprompt#1379 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 14, 2026
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-objstore-allowlist-twin branch from 323ffab to 9f32f89 Compare October 14, 2026 20:13

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced at 9f32f89 on PG18. objstore_allowlist is 16/16. With the GUC default changed to 127.0.0.1, it's 13 + 3: the list defaults to empty: got [set: [127.0.0.1]] want [empty], plus the two default arms that depend on it. The check now produces empty positively, so a dead session can't pass it. Approving. I'll merge once CI is green, after checking the approval is on this head.

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…d the shell names 'empty' itself (commandprompt#1379 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-objstore-allowlist-twin branch from 9f32f89 to 02ad067 Compare October 15, 2026 16:27
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
@OffgridwithJD

OffgridwithJD commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto 57f6e0e (#1401), on top of #1403. The new head is 02ad067. Only the count files changed, so this needs your re-approval.

  • Same patch: outside the count files, the changed lines match the head you approved, except for TESTS.md section renumbers where main's new sections took the old numbers.
  • The ledger is restated on main's census and agrees with its tsv: 2802 rows, 2104 never, 698 dated, read back from the file. Each changed paragraph's arithmetic starts from main's last numbers.
  • cluster_tests 567, by collection over the cluster files on this tree. guard_tests 433 is unchanged.
  • Runs on the rebased tree at 217f23b, PG17 and PG18: PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG17 objstore_allowlist: 16 passed + 0 failed + 0 unrunnable + 0 skipped; PG17 pytest: 41 passed in 9.60s; PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 objstore_allowlist: 16 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 pytest: 41 passed in 9.11s. harness_selftest 1245/1245 and docs_style passed on both. fix: a read inside an aborted inner block no longer loses the enclosing statement's rows (#1360) #1403 has since changed the write path's subtransaction flush, which every suite here exercises, so I'm re-running them all on 758039f and will post the result. CI runs PG17 and PG18 on this head.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Re-run finished on the tree after #1403 (758039f or later). Everything is green, so the head above stands:

  • PG17 docs_style: passed
  • PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG17 objstore_allowlist: 16 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG17 pytest: 42 passed in 8.31s
  • PG17 vacuum_lock_privilege: 19 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 docs_style: passed
  • PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 objstore_allowlist: 16 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 pytest: 42 passed in 8.33s
  • PG18 vacuum_lock_privilege: 19 passed + 0 failed + 0 unrunnable + 0 skipped

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…d the shell names 'empty' itself (commandprompt#1379 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-objstore-allowlist-twin branch from 02ad067 to 72b5f34 Compare October 15, 2026 17:06
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto 7da848d (#1432, #1409). The new head is 72b5f34; only the count files changed, so please re-approve this head.

🤖 Generated with Claude Code

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved at 72b5f34. Compared with 9f32f89, the head I approved, file by file: the changed lines are the same, apart from the count files (check_ledger_budget.txt, expected_tests.txt) and, in #1378/#1379/#1380/#1383, the TESTS.md section number, now 115.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto 2d1a77f (#1413–#1424). The new head is ea3278f, so please re-approve this head.

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-objstore-allowlist-twin branch from 9be2896 to ea3278f Compare October 15, 2026 20:58
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…d the shell names 'empty' itself (commandprompt#1379 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved at ea3278f, the rebase onto 2d1a77f. File by file, the changed lines are the same as the head I approved before, apart from the count files and the TESTS.md section number.

🤖 Generated with Claude Code

jdatcmd and others added 21 commits October 15, 2026 21:15
…ws are reworded

The mutation column is a ';'-separated set, written back sorted. A --mutation
whose text held "; " was stored as two mutations, one a fragment starting with
a space, and every later merge re-sorted it. commandprompt#1427's recovery_minimal_wal text
did this to 8 rows, and commandprompt#1423, commandprompt#1424 and commandprompt#1426 each carried them as a change of
their own. Four older harness_selftest rows were split the same way.

- merge refuses a --mutation holding ';' (rc 2, ledger untouched).
- the reader refuses a ';'-piece with whitespace at an end, by line: the writer
  joins with a bare ';', so such a piece is a fragment, never a mutation.
- the 12 rows are reworded without ';', from their original text in history.
  Only field 6 changes; the census stays 3007 / 2282 / 725.

Red first: the two refusal arms got 0 want 2 on the unchanged tool, and the
committed-ledger arm was red at line 1783 with the refusal in and the rows not
yet reworded. guard_tests 435 -> 438 by collection.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ment-mark mutation (commandprompt#1341 item 75)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…wn privileges, in both twins (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…d the refused tier writes no file, in both twins (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
… back; both its branches echoed "ran" (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…nd every owner arm reports a number when refused, in both twins (commandprompt#1341)

A guard widened to refuse the owner stopped the fixture's own export, so a
premise reddened for the product code and the pytest twin crashed in setup
rather than failing an arm by name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e mutations of the RLS guard (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…x, and the twin fails it by name rather than raising (commandprompt#1383 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…name (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… arm runs with the configured list empty, in both twins (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n comparing two empty strings (commandprompt#1341)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d the shell names 'empty' itself (commandprompt#1379 review)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Restated for the chain you asked for. The new head is b0303e6, so please re-approve this head.

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-objstore-allowlist-twin branch from ea3278f to b0303e6 Compare October 15, 2026 21:23

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved at b0303e6, its place in the chain on 17420be. This PR's own commits, measured from the link below it, change the same lines as the head I approved before, apart from the count files and the TESTS.md section number.

🤖 Generated with Claude Code

@jdatcmd
jdatcmd merged commit 2c72984 into commandprompt:main Oct 15, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants