8000 T52 — Add the dynamic_ingress setting to the box's ingress policy by minimal-foundry[bot] · Pull Request #1699 · gominimal/minimal · GitHub
[go: up one dir, main page]
Skip to content

T52 — Add the dynamic_ingress setting to the box's ingress policy - #1699

Merged
norrietaylor merged 6 commits into
mainfrom
agent/1625-add-the-dynamic-ingress-setting-to-the-box-s-ing
Sep 25, 2026
Merged

T52 — Add the dynamic_ingress setting to the box's ingress policy#1699
norrietaylor merged 6 commits into
mainfrom
agent/1625-add-the-dynamic-ingress-setting-to-the-box-s-ing

Conversation

@minimal-foundry
@minimal-foundry minimal-foundry Bot commented Sep 25, 2026
Copy link
Copy Markdown
Contributor

T52 — Add the dynamic_ingress setting to the box's ingress policy

slice: S13
kind: enablement
covers: none
size: M
est: 100 code, 120 tests
executor: agent
difficulty: routine
model: standard
diagnostics: none: the bundle already carries every session record, where the setting lands
observability: none: a policy field emits nothing until the expose request reads it
layers:
- schema:
- crates/sessions/src/lib.rs: IngressPolicy gains dynamic_ingress with its validation
test: cargo nextest run -p sessions dynamic_ingress_setting_parses
- literals:
- crates/minimal/src/cmd/session.rs: its IngressPolicy literal follows
- crates/minimald/src/net/switch.rs: its IngressPolicy literals follow
- crates/minimald/tests/netns_root_integration.rs: its IngressPolicy literal follows
- crates/minimald-rpc/src/lib.rs: its IngressPolicy literal follows
- crates/minimal-tui/tests/snapshots.rs: its IngressPolicy literal follows
- crates/minimal-tui/src/render.rs: the detail pane shows the setting
test: cargo nextest run -p minimal-tui detail_pane_shows_dynamic_ingress
produces: none
consumes: none
proof: A box spec declaring dynamic_ingress allow, deny or ask parses, an unknown value is refused, and the TUI's detail pane shows the setting.

Proof

A box spec declaring dynamic_ingress allow, deny or ask parses; an unknown value is refused by serde; and the TUI detail pane renders the setting as 'dynamic ingress '.

Notes from the agent: Also re-exported DynamicIngress through minimald-rpc so wire consumers can name the type. No files outside the task's layers were modified.

Tests

line exit tail
cargo nextest run -p sessions dynamic_ingress_setting_parses 0 Applying loadouts: default Running task runtime-implement-test-0 in session task-runtime-implement-test-0-3cc4... Compiling args v0.0.1 (/home/runner/.local/state/minimal/sessions/0fdf2/tree/crates/args) Compiling ot v0.0.1 (/home/runner/.local/state/minimal/sessions/0fdf2/tree/crates/ot) C
cargo nextest run -p minimal-tui detail_pane_shows_dynamic_ingress 0 Applying loadouts: default Running task runtime-implement-test-1 in session task-runtime-implement-test-1-339e... Compiling args v0.0.1 (/home/runner/.local/state/minimal/sessions/7466a/tree/crates/args) Compiling ot v0.0.1 (/home/runner/.local/state/minimal/sessions/7466a/tree/crates/ot) C

Verify

min task run agent-verify passed ([tasks.verify]).

Out of scope

These files are outside the task's layers:, so this PR is a draft:

  • crates/minimal-tui/tests/snapshots/snapshots__detail_pane_shows_dynamic_ingress.snap

Closes #1625

@minimal-foundry
feat(sessions): add dynamic_ingress setting to IngressPolicy
d96ee58 
Adds a DynamicIngress enum (allow/deny/ask) to the session crate's
IngressPolicy and threads the new field through every literal site:
minimal activate, minimald-rpc round-trip tests, minimald switch and
netns integration tests, and the TUI detail pane.

The setting serializes as snake_case, validates only via serde (unknown
values are refused at parse time), and the default remains deny-all when
absent. The TUI policy section now shows the dynamic ingress mode
beside static mappings and dynamic ports.

tests:
- cargo nextest run -p sessions dynamic_ingress_setting_parses
- cargo nextest run -p minimal-tui --test snapshots detail_pane_with_policy
@minimal-foundry
fix(minimal-tui): rename detail pane test to match task test line
1b0d595 
The test that exercises the dynamic_ingress detail pane rendering was
named detail_pane_with_policy, so cargo nextest run -p minimal-tui
detail_pane_shows_dynamic_ingress found no tests to run. Rename the
test and its snapshot to the name the task's test line expects.
@minimal-foundry minimal-foundry Bot mentioned this pull request Sep 25, 2026
Closed
@coderabbitai
coderabbitai Bot commented Sep 25, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 307a3bfd-63fd-44dc-b2ea-046438759956

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@minimal-foundry minimal-foundry Bot mentioned this pull request Sep 25, 2026
Open
@norrietaylor
norrietaylor commented Sep 25, 2026
Copy link
Copy Markdown
Member

Babysitter review of T52 against the plan block in #1625, at 1b0d595.

Verdict: revise, one display gap. All seven layers: intents are met; both test lines are real tests that exit 0 and neither self-skips; the spelling dynamic_ingress with allow|deny|ask matches the spec's field-name section and NET-043 to NET-045; records and replies without the field decode as None, and IngressPolicy ignores unknown keys for older readers. The single out-of-layer file is the snapshot the required TUI test produces, so the draft is procedural and I will un-draft once the round below lands.

Medium crates/minimal/src/cmd/session.rs:976: min session policy decides "deny all" from port_mappings and dynamic_allowed_range alone and never prints dynamic_ingress. A policy whose only ingress setting is dynamic_ingress = allow prints ingress / deny all, which is wrong on the command people use to inspect a box's rules. The dash detail pane (render.rs:546-563) and IngressPolicy::is_empty already handle the field.

Low

  • crates/sessions/src/lib.rs:536,597: an explicit dynamic_ingress = deny is the documented default, yet declaring it on a host-address or no-network box is refused with IngressRequiresOwnIp because is_empty counts any Some. The test covers this only with ask.
  • The enum carries #[default] Deny while the field is Option, so "default" has two spellings that render differently.
  • detail_pane_with_policy was renamed rather than kept, so nothing now snapshots the pane without dynamic ingress.
  • The test comment cites NET-043 as covered; the plan says covers: none, and only JSON input is parsed.

@norrietaylor
norrietaylor commented Sep 25, 2026
Copy link
Copy Markdown
Member

/revise Fix the min session policy display and two small edges, all in files the task already owns:

  1. crates/minimal/src/cmd/session.rs:976: include dynamic_ingress in the "deny all" decision (&& ingress.dynamic_ingress.is_none()) and print a dynamic ingress {mode} row when it is set, mirroring the dash detail pane in render.rs:546-563. Add a unit test in that file that a policy with only dynamic_ingress = allow prints the row and not deny all.
  2. crates/sessions/src/lib.rs: an explicit dynamic_ingress = deny is the default and must not be refused on a host-address or no-network box. Make IngressPolicy::is_empty treat Some(Deny) like None, and extend egress_on_none_box_is_validation_error's sibling test (or add one beside dynamic_ingress_setting_parses) to show deny is accepted on a none box while allow and ask are refused.
  3. crates/minimal-tui: restore the original detail_pane_with_policy snapshot test alongside detail_pane_shows_dynamic_ingress so the pane without dynamic ingress stays covered.

Do not touch anything else. Keep both test lines passing.

@minimal-foundry
minimal-foundry Bot commented Sep 25, 2026
Copy link
Copy Markdown
Contributor Author

Round 1 of 10 on a /revise note, pushed as 7d21a13022df.

line exit tail
cargo nextest run -p sessions dynamic_ingress_setting_parses 0 Applying loadouts: default Running task runtime-implement-test-0 in session task-runtime-implement-test-0-d570... Compiling args v0.0.1 (/home/runner/.local/state/minimal/sessions/e1365/tree/crates/args) Compiling ot v0.0.1 (/home/runner/.local/state/minimal/sessions/e1365/tree/crates/ot) C
cargo nextest run -p minimal-tui detail_pane_shows_dynamic_ingress 0 Applying loadouts: default Running task runtime-implement-test-1 in session task-runtime-implement-test-1-0393... Compiling args v0.0.1 (/home/runner/.local/state/minimal/sessions/4824f/tree/crates/args) Compiling ot v0.0.1 (/home/runner/.local/state/minimal/sessions/4824f/tree/crates/ot) C

Verify min task run agent-verify passed.

Outside the task's layers::

  • crates/minimal-tui/tests/snapshots/snapshots__detail_pane_shows_dynamic_ingress.snap

Converted to a draft: something above needs a person before review.

A box spec declaring dynamic_ingress allow, deny, or ask parses; unknown values are refused; an explicit deny is accepted as the default on a host-address or no-network box, while allow and ask are refused there; and min session policy now prints the dynamic ingress mode (mirroring the TUI detail pane). The TUI retains both the original policy snapshot and the dynamic-ingress snapshot.

Notes from the agent: Three commits on the agent branch: (1) IngressPolicy::is_empty now treats Some(Deny) as empty (matching its own Default), with validation tests confirming deny is allowed on HostNet/NoNet while allow/ask are rejected. (2) Restore detail_pane_with_policy snapshot test alongside detail_pane_shows_dynamic_ingress. (3) format_policy includes dynamic_ingress in the deny-all decision and prints a dynamic-ingress row, with unit tests verifying it prints the row and not deny-all when only dynamic_ingress=allow is set, and that it renders alongside static mappings.

@norrietaylor
norrietaylor marked this pull request as ready for review September 25, 2026 08:27
@norrietaylor
norrietaylor requested a review from a team as a code owner September 25, 2026 08:27
@minimal-foundry
minimal-foundry Bot commented Sep 25, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

Approved: no findings at 6524ac54f5c3.

Nothing to do.

Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it.

Clobber: none: git log on the touched files shows only the agent's own fix commits (5d9d526, 18a4ec3, 7d21a13), no reverted upstream fix.

minimal-foundry[bot]
minimal-foundry Bot commented Sep 25, 2026
View reviewed changes
@minimal-foundry minimal-foundry Bot left a comment
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

minimal-review — advisory review

Verdict COMMENT — 1 finding, 1 inline.

Comment thread crates/sessions/src/lib.rs Resolved
minimal-foundry[bot]
minimal-foundry Bot commented Sep 25, 2026
View reviewed changes
@minimal-foundry minimal-foundry Bot left a comment
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

minimal-review — approved

Verdict APPROVE — No findings.

@minimal-foundry minimal-foundry Bot added the review:approved minimal-review approved the current head label Sep 25, 2026
@minimal-foundry
minimal-foundry Bot commented Sep 25, 2026
Copy link
Copy Markdown
Contributor Author

Round 2 of 10 on the runtime's review findings: the coder changed nothing. every commit this round put on the branch above 7d21a13022df is a merge of the base branch (6524ac54f5c3), so the pull request carries no work from it.

norrietaylor
norrietaylor approved these changes Sep 25, 2026
View reviewed changes
@norrietaylor norrietaylor left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed against the T52 plan block; my revise (7d21a13) verified, round 2 changed nothing, the one LOW thread was auto-resolved on 6524ac5. All five lane aggregators, minimal-review and CodeRabbit green. Auto-merge.

@norrietaylor
norrietaylor merged commit e36343f into main Sep 25, 2026
33 checks passed
@norrietaylor
norrietaylor deleted the agent/1625-add-the-dynamic-ingress-setting-to-the-box-s-ing branch September 25, 2026 09:07
@minimal-foundry minimal-foundry Bot mentioned this pull request Sep 25, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@norrietaylor norrietaylor norrietaylor approved these changes

Assignees

No one assigned

Labels

review:approved minimal-review approved the current head

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

T52 — Add the dynamic_ingress setting to the box's ingress policy

1 participant

@norrietaylor
0