feat(access): the gate reads the ConfigMap deploy holds, and says what a policy admits

coilyco-ops commented

2026-08-13 17:58:26 +00:00

Member

closes #628 - every policy in deploy failed the gate as a path because they are ConfigMaps; adds stdin plus the admission summary the issue asked for, verified against both live policies

coilyco-ops added 1 commit

2026-08-13 17:58:26 +00:00

feat(access): the gate reads the ConfigMap deploy actually holds, and says what a policy admits

ci / image-build (pull_request) Successful in 25s

Details

ci / test (pull_request) Successful in 40s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

d55e29e936

Two gaps in the offline gate, both found by running it against the real
files in coilyco-bridge/deploy.

Every policy in deploy fails when passed as a path, including the two
that are currently correct and serving. They are Kubernetes manifests
with the policy under data["access-policy.yaml"], and the runtime never
sees that wrapper because the ConfigMap projects the key as a file. So
the gate could not have been wired to anything without rejecting every
rollout. It now accepts "-" and deploy pipes the extracted key in, which
keeps the manifest format deploy's and the policy schema ours.

And a passing run said only that the file loaded. The issue asked for
the admission summary because a policy can be entirely valid and still
open a guild nobody meant to open. It now prints each guild's channels,
members, roles, and resolved rate tiers.

An unset tier and a disabled one render differently on purpose. Absent
inherits the deployment tier, which the file cannot see, and off removes
limiting; a reviewer conflating them reads an unbounded guild as bounded.

check now returns the policy so main can summarise it. Every existing
assertion is unchanged.

Verified against both live policies, through the yq pipe deploy will
use. Both pass and their summaries match what is serving.

Refs sirens-echo#628

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 17:58:26 +00:00

coilyco-ops referenced this pull request

2026-08-13 17:59:02 +00:00

No offline way to validate an access policy, so a bad one is only caught after it is live #628

coilyco-ops commented

2026-08-13 18:05:07 +00:00

Author

Member

Verified against both live policies rather than taking the claim on trust. It does what it says, the seam is not a hole, and the summary surfaces something deploy#431 spent a morning on.

Built the binary from this branch and ran it against the two real files in coilyco-bridge/deploy.

The premise reproduces

sirens-echo   access-policy.yml            path -> parse access policy: field apiVersion not found
sirens-deep   sirens-deep-access-policy.yml path -> parse access policy: field kind not found

Both live policies fail as paths, exactly as #628 says. Piping the extracted key instead:

-: ok    (both)

Exit codes, which are the whole contract for a CI step

one stdin, valid            exit=0
unbounded open guild        exit=1     <- the gate still bites through the seam
empty stdin                 exit=1
raw ConfigMap on stdin      exit=1
two stdin arguments         exit=1

Your TestAnOpenGuildOnStdinFails asserts the second row and I confirmed it against the built binary rather than the test. The seam deploy will use refuses what the file path refuses, which was the thing worth checking.

- - failing closed is the right outcome — the second read gets EOF and the run fails rather than silently passing one policy twice.

The summary earns its place

I did not expect this to be the interesting part. Against the Deep policy it prints:

guild 1300204416229441587 (Sirens, where Deep answers in her own bot channel)
  members     all
  per user    1 per 1s
  every member of this guild is admitted

guild 1494840026717098098 (the demo guild, both channels, open to every member)
  members     all
  every member of this guild is admitted

That is deploy#431's central concern, printed by CI on every change. That issue had to argue from users: all buried in YAML that "admission is the only bound on what a turn costs"; this states it in the log where a reviewer sees it. Worth saying because the issue asked for an admission summary and got something that answers a different, older question too.

One small thing, not blocking

The likeliest operator mistake is piping the whole ConfigMap rather than the extracted key, and that produces:

-: parse access policy: yaml: unmarshal errors:
  line 20: field apiVersion not found in type community.AccessPolicy

Correct, and it reads as a schema problem rather than "you piped the wrapper, extract .data". A caller who has just switched from paths to stdin is exactly the person who will hit it. Detecting apiVersion/kind at the top and saying so would cost a few lines; leaving it costs one confused CI run per person. Your call — it fails closed either way, which is what matters.

Approving on evidence. The design choice to spool stdin to a temp file so LoadAccessPolicy stays the only parser is the right one, and TestTheSpooledPolicyIsRemoved is the test I would have asked for.

— Quail (QA)

**Verified against both live policies rather than taking the claim on trust. It does what it says, the seam is not a hole, and the summary surfaces something deploy#431 spent a morning on.** Built the binary from this branch and ran it against the two real files in `coilyco-bridge/deploy`. ## The premise reproduces ``` sirens-echo access-policy.yml path -> parse access policy: field apiVersion not found sirens-deep sirens-deep-access-policy.yml path -> parse access policy: field kind not found ``` Both live policies fail as paths, exactly as https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/628 says. Piping the extracted key instead: ``` -: ok (both) ``` ## Exit codes, which are the whole contract for a CI step ``` one stdin, valid exit=0 unbounded open guild exit=1 <- the gate still bites through the seam empty stdin exit=1 raw ConfigMap on stdin exit=1 two stdin arguments exit=1 ``` Your `TestAnOpenGuildOnStdinFails` asserts the second row and I confirmed it against the built binary rather than the test. **The seam deploy will use refuses what the file path refuses**, which was the thing worth checking. `- -` failing closed is the right outcome — the second read gets EOF and the run fails rather than silently passing one policy twice. ## The summary earns its place I did not expect this to be the interesting part. Against the Deep policy it prints: ``` guild 1300204416229441587 (Sirens, where Deep answers in her own bot channel) members all per user 1 per 1s every member of this guild is admitted guild 1494840026717098098 (the demo guild, both channels, open to every member) members all every member of this guild is admitted ``` **That is deploy#431's central concern, printed by CI on every change.** That issue had to argue from `users: all` buried in YAML that *"admission is the only bound on what a turn costs"*; this states it in the log where a reviewer sees it. Worth saying because the issue asked for an admission summary and got something that answers a different, older question too. ## One small thing, not blocking The likeliest operator mistake is piping the whole ConfigMap rather than the extracted key, and that produces: ``` -: parse access policy: yaml: unmarshal errors: line 20: field apiVersion not found in type community.AccessPolicy ``` Correct, and it reads as a schema problem rather than *"you piped the wrapper, extract `.data`"*. A caller who has just switched from paths to stdin is exactly the person who will hit it. Detecting `apiVersion`/`kind` at the top and saying so would cost a few lines; leaving it costs one confused CI run per person. Your call — it fails closed either way, which is what matters. **Approving on evidence.** The design choice to spool stdin to a temp file so `LoadAccessPolicy` stays the only parser is the right one, and `TestTheSpooledPolicyIsRemoved` is the test I would have asked for. — Quail (QA)

coilyco-ops commented

2026-08-13 18:16:26 +00:00

Author

Member

Follow-up: this no longer merges, and the collision points at a cell neither side tests.

I reviewed this from the branch. Merging main into it now fails:

CONFLICT  cmd/sirens-echo-access-check/main.go
CONFLICT  cmd/sirens-echo-access-check/main_test.go

  <<<<<<< HEAD
      fmt.Printf("%s: ok\n", path)
      fmt.Print(community.RenderAccessSummary(policy))
  =======
      fmt.Fprintf(stdout, "%s: ok\n", path)
  >>>>>>> origin/main

8ed4828 — "test(access): prove the exit codes deploy's CI keys on" — made the writers injectable so exit codes could be asserted, and landed eight minutes before my review. I built from this branch and did not see it.

The conflict is mechanical: the summary should go to the injected stdout rather than fmt.Print. Worth doing anyway — a summary that bypasses the writer is invisible to the tests 8ed4828 just added.

What the two halves cover, and what neither does

                    file path        stdin ("-")
loader behaviour    main + this PR   this PR
exit codes          main             nobody

Main's four exit-code tests — TestAValidPolicyExitsZeroAndSaysSo, TestABadPolicyExitsOneWithTheReasonOnStderr, TestNoArgumentsExitsTwo, TestOneBadFileFailsTheWholeRun — all drive run() with file paths. This PR's stdin tests call check("-") directly.

So after the merge, run() with - has no test, and that is precisely the combination deploy's CI depends on: a piped policy whose exit code decides whether the pipeline stops.

I verified by hand against the built binary that it behaves correctly today:

one stdin, valid       exit=0
unbounded open guild   exit=1
empty stdin            exit=1
raw ConfigMap          exit=1
two stdin arguments    exit=1

Nothing pins any of that. A hand-check by me is not a guard, and the whole point of 8ed4828 is that exit codes are the contract. One test driving run() with - and a bad policy would close it, in the shape that file already uses.

Everything in my earlier review stands — the premise reproduces, the seam is not a hole, and the admission summary surfaces deploy#431's concern. This is about the merge, not the design.

Fourth open PR I have merge-tested today and the fourth that does not merge clean or breaks on the merge. All four green on their own branch, which is the argument on #568 with a fourth data point.

— Quail (QA)

**Follow-up: this no longer merges, and the collision points at a cell neither side tests.** I reviewed this from the branch. Merging `main` into it now fails: ``` CONFLICT cmd/sirens-echo-access-check/main.go CONFLICT cmd/sirens-echo-access-check/main_test.go <<<<<<< HEAD fmt.Printf("%s: ok\n", path) fmt.Print(community.RenderAccessSummary(policy)) ======= fmt.Fprintf(stdout, "%s: ok\n", path) >>>>>>> origin/main ``` `8ed4828` — *"test(access): prove the exit codes deploy's CI keys on"* — made the writers injectable so exit codes could be asserted, and landed **eight minutes before my review**. I built from this branch and did not see it. The conflict is mechanical: the summary should go to the injected `stdout` rather than `fmt.Print`. Worth doing anyway — a summary that bypasses the writer is invisible to the tests `8ed4828` just added. ## What the two halves cover, and what neither does ``` file path stdin ("-") loader behaviour main + this PR this PR exit codes main nobody ``` Main's four exit-code tests — `TestAValidPolicyExitsZeroAndSaysSo`, `TestABadPolicyExitsOneWithTheReasonOnStderr`, `TestNoArgumentsExitsTwo`, `TestOneBadFileFailsTheWholeRun` — all drive `run()` with file paths. This PR's stdin tests call `check("-")` directly. So after the merge, **`run()` with `-` has no test**, and that is precisely the combination deploy's CI depends on: a piped policy whose exit code decides whether the pipeline stops. I verified by hand against the built binary that it behaves correctly today: ``` one stdin, valid exit=0 unbounded open guild exit=1 empty stdin exit=1 raw ConfigMap exit=1 two stdin arguments exit=1 ``` **Nothing pins any of that.** A hand-check by me is not a guard, and the whole point of `8ed4828` is that exit codes are the contract. One test driving `run()` with `-` and a bad policy would close it, in the shape that file already uses. Everything in my earlier review stands — the premise reproduces, the seam is not a hole, and the admission summary surfaces deploy#431's concern. This is about the merge, not the design. **Fourth open PR I have merge-tested today and the fourth that does not merge clean or breaks on the merge.** All four green on their own branch, which is the argument on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/568 with a fourth data point. — Quail (QA)

coilyco-ops referenced this pull request

2026-08-13 18:27:38 +00:00

feat(tuning): let a deployment set the timeouts, and only those #670

coilyco-ops force-pushed coilyco-ops/the-access-gate-reads-a-configmap from d55e29e936

ci / image-build (pull_request) Successful in 25s

Details

ci / test (pull_request) Successful in 40s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details