feat(marking): poc (#7510) - #7618
Draft
corinnekrych (corinnekrych) wants to merge 25 commits into
Draft
Conversation
…7510) Implements steps 1.2 and 1.3 of the marking design: the is_marking_missing SQL function and a MarkingDimension that emits a correlated anti-join against a per-table <table>_markings join table. This is the Option 1 schema shape. It is committed as the fallback of record before step 1.4 evaluates the marking_ids column shape (Option 2), so the working implementation stays recoverable if that spike fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 1.4 — the schema-shape spike that the Option 2 decision was gated on.
Replaces the per-table `<table>_markings` join table with a `marking_ids
text[]` column on the marked row, and the correlated anti-join
`is_marking_missing(id)` with a local containment test
`is_marking_set_allowed(marking_ids)`.
All five spike criteria hold, pinned by MarkingRewriteHypothesisTest:
1. the truth table is unchanged — unmarked visible, in-clearance visible,
out-of-clearance hidden, multi-marked rows need every marking;
2. fail-closed on GUC unset, GUC empty, NULL array and '{}' array;
3. the `&&`-on-lacked form is demonstrated to leak, so the GIN-friendly
"optimisation" is a failing red line rather than a plausible refactor;
4. a composite-PK relationship table is markable — the property the join
table could not deliver, and 63 tables in the schema have composite PKs;
5. the plan is measured, not guessed: the SQL function is inlined, GIN is
skipped on selectivity grounds rather than opacity, and the residual
cost is ~0.25us/row of GUC parsing.
Eliminates risk 6.5: with the markings on the marked row there is no
second relation through which an invisible row's markings can be read.
Gives up FK integrity (risk 6.8), recovered by the write guard on the
insert side and an array_remove scrub generated from MarkedTables on the
delete side.
Q10 still holds — the new predicate adds zero bind parameters, so tenant
v1 @filter placeholder positions are preserved.
The 79 TenantStatementInspectorTest cases stay green and unmodified.
Option 1 remains recoverable at 7056fd6 if this is ever reverted.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gives the marking PoC its vocabulary: a per-tenant table of classification levels (TLP, PAP, or a tenant own scale) that a clearance can later be granted in and a record tagged with. This is step 2.1 of the Option C design, deliberately skimmed - no capability chain, no assignment UX. Tenant isolation is v2 (statement inspector + can_access_tenant) rather than v1 @filter: the table is brand new, so there are no legacy read paths to migrate, and step 2.3 composes the marking dimension on top of the same inspector. groups_markings is created as schema only. Its assign/unassign endpoint is not built here because nothing consumes it until 2.2, where a test can prove it works. The name-uniqueness check uses a list-returning finder: the unique index is composite on (marking_name, tenant_id), so several tenants legitimately own TLP:RED and a single-result finder would throw as soon as the table were not inspector-scoped. Frontend CRUD under Settings > Security > Marking definitions was pulled forward from step 5.6 so the vocabulary is visible while step 3 is designed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Corinne Krych <corinne.krych@filigran.io>
Step 2.2 of the marking access-control PoC: derive a user's marking clearance from their group grants, and cache it per (user, tenant, bypass). MarkingCtx mirrors TxCtx but its empty state is None, not Missing: an empty clearance is a normal, safe state, since unmarked rows remain visible to it. Fail-closed for markings means "see less", not "see nothing". MarkingScopeResolver collapses ordinality in Java — max order per type, expanded back to every id at or below it — so the SQL predicate stays a plain containment test. A type with no grant contributes nothing, not its lowest level. BYPASS is expanded to an explicit id list at resolution time rather than carried onward as a wildcard. MarkingClearanceCacheManager reads over raw JDBC with @AllowRawJdbc: like TenantMembershipCacheManager it runs on the pre-transaction argument-resolver path, where a JPA query would pin a pool connection for the whole request. Both statements bind tenant_id explicitly to replace the inspector it bypasses, pinned by MarkingClearanceCacheManagerTest.SqlExemption. evict drops both bypass variants: naming only one would let a stale, wider entry survive under the other key, and a stale larger clearance fails open. Nothing wires evict yet — no reader of the GUC exists before step 3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The marking design lived in .claude/, which is ignored by contributors' global gitignore, so it could not be reviewed. Move it into brainstorming/ and summarise the decision as an ADR. ADR-007 follows ADR-002, which is the direct parent: marking generalises the same statement-inspector enforcement point into a second scope dimension. It records why Option C wins on the "enforcement must not be forgettable" driver, and is deliberately left Proposed until the PoC definition of done is met. brainstorming/ separates the reasoning from the decision: ADR is reviewed and stable, brainstorming is honest and current, docs/docs/development documents what already exists. README states the split so the next feature does not have to rediscover it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A marking clearance is cached for 5 minutes, so any change that REDUCES one is fail-open until it expires: the user keeps reading rows they are no longer cleared for. Wire eviction into every mutation that can shrink a clearance. Adds evictForUser / evictForUsers alongside the existing evict, and calls them from group membership changes, group deletion and marking definition update/delete. Three decisions worth recording: - evict(userId, tenantId) is the wrong API for membership changes. Group implements DualScopeBase, so a platform group (tenant_id IS NULL) grants into many tenants at once and users_groups carries no tenant of its own. Evicting one tenant leaves the others stale - the exact fail-open case eviction exists to prevent. evictForUser walks every tenant the user belongs to. - evict drops BOTH bypass variants. Making the caller name the right one would let a stale, larger entry survive under the other key. - MarkingDefinitionService.update evicts ALL, not a targeted set: order and type are resolver INPUTS, not labels. Raising a marking's order pushes it above clearances that previously covered it, and the affected set is "everyone holding a grant of this type". Deliberately NOT wired: asset marking writes. is_marking_set_allowed takes the row's array as a function argument and re-reads it every query; only the clearance lives in the cached GUC. An evict there would be a no-op that looks like protection, which is worse than none. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sets app.current_markings next to app.current_tenants on the HTTP path and the background path. The GUC is written but nothing reads it yet - no table is marking-active - so this is inert until activation. The background half is the point. Skipping it would not fail a test; it would make every collector, executor and the ES sync silently read a subset the moment a table is activated. TenantScopedTransaction now resolves a system clearance (all markings of the tenants in scope - a scheduler is not a user) in the same setScope call that writes the tenant GUC. Introduces MarkingScopeSupplier as a seam: the aspect lives in openaev-model but clearance derivation needs UserService and the clearance cache, both in openaev-api. The alternatives were a second aspect or resolving in TxCtxArgumentResolver; the seam won because the invariant worth protecting is that ONE component owns "what scope may this transaction have". A second aspect makes ordering load-bearing, and the argument resolver splits scope arrival across two routes. ObjectProvider keeps the supplier optional so model-only slices still start. Tenant is passed, clearance is derived. A caller legitimately chooses which tenant to act in; nobody chooses their own clearance. That is why there is no X-Markings header and never should be. Note the asymmetry, which is the easiest thing here to misread: TxCtx.Missing means zero rows, but MarkingCtx.None still shows UNMARKED rows - the empty set is contained in the empty set. Fail-closed for marking means "see less", never "see nothing" and never "see more". Known blast radius for activation: a @transactional method with no TxCtx parameter writes neither GUC, so it will see only unmarked rows once a table is active. Silent partial narrowing, not an error. Documented in the aspect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PUT /api/tenants/{t}/groups/{g}/markings - the write path that gives a user
a clearance. Replace-the-whole-set, like the users and roles endpoints next
to it: an empty list revokes every grant. A PATCH-style add/remove would
make "what does this group grant?" depend on request ordering, which is the
wrong property for a security boundary.
This was the gate on activating any table. Until it existed nothing wrote
groups_markings, so every user resolved to MarkingCtx.none() and the whole
clearance path could only be exercised against a stubbed JdbcTemplate. The
step-3 goal - "a user cleared TLP:GREEN cannot read a TLP:RED asset" - is
undemonstrable with an empty grant table.
MarkingEscalationValidator is the guard that makes marking a boundary at
all: without it, anyone able to manage a group could put themselves in it,
grant it TLP:RED and read everything - the capability to manage groups would
silently become the capability to read every marked row. It checks the
RESOLVED clearance, not the raw grants, so a user holding TLP:AMBER may
grant TLP:GREEN: they can already read every GREEN row, so granting it
discloses nothing they could not disclose otherwise.
The read path for a clearance stays raw JDBC (OSIV/Hikari); only the write
path uses the ORM, where no such constraint applies.
Finding, recorded in the arch-test allowlist and in the test: tenant
isolation cannot be the ONLY guard on a cross-tenant assignment. The
statement inspector rewrites queries - it cannot filter a read that is never
ISSUED, and an entity already in the persistence context is served from
Hibernate's first-level cache. The escalation guard is the independent
guarantee, since a clearance is per tenant. The two are not redundant, and
this generalises to every tenant-active table.
Still deferred: the ASSIGN_MARKING capability chain (the endpoint reuses the
group's own WRITE control) and the platform-group equivalent, which is a
cross-tenant question this PoC should not answer by accident.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#7510) EndpointSpecification used GROUP BY(id) as a DISTINCT in disguise: with a full column projection that is only legal because PostgreSQL infers a functional dependency from the table's primary key. That inference is fragile in a way that matters here. A scope-filtered table is rewritten by the statement inspector into an inline view, and an inline view carries no primary key - so the dependency can no longer be inferred and every such query fails with: column "e1_0.ai_target_configuration" must appear in the GROUP BY clause Reproduced directly in psql: the same query is accepted against the bare table and rejected against "(SELECT * FROM assets WHERE ...) AS e1_0". DISTINCT expresses the actual intent - drop the rows multiplied by the LEFT JOIN - and survives the rewrite, since every projected column is comparable (the two jsonb columns have equality). findAgentlessEndpoints drops the de-duplication entirely: isEmpty() compiles to a subquery, not a join, so no rows were ever multiplied. Latent until now because no table this query touches was scope-active. It becomes load-bearing the moment assets is. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The first marking-enabled table, and the proof of the design's central
claim: a user cleared TLP:GREEN cannot see a TLP:RED endpoint, with NO
change to EndpointRepository or EndpointService read code. The filtering
happens because assets is on openaev.marking.active-tables and the statement
inspector rewrites the SQL underneath.
AssetMarkingIsolationTest calls POST /api/endpoints/search and
GET /api/endpoints/{id} exactly as any other test does. Mutation-checked:
with the dimension switched off, 5 of its 7 tests fail - RED becomes
visible and the 404 becomes a 200 - so the tests bite.
An earlier version of this message claimed "5842 tests, 0 failures" with
the table active. That measurement was invalid and the claim is withdrawn.
openaev-api/src/test/resources/application.properties SHADOWS the main
application.properties - Spring Boot loads the first one on the classpath
and does not merge them - so the property set in main was never in force
during that run. The suite was green because the dimension was OFF.
With the property actually set in the test profile the real number is
6047 tests, 6 failures, 43 errors, from three distinct causes: the
inspector's inline view destroying primary-key functional dependency in
GROUP BY, a JSqlParser limitation on lower() over a concatenation
containing a bind parameter, and a table function the fail-closed
inspector refuses. Each is fixed in a following commit, with the test
property flipped once the tree is green again. The blast radius of
activation is now measured rather than assumed - and the lesson is that
a dimension can appear inert simply because it was never switched on.
Schema is one nullable text[] column. No join table, no foreign key, no
cascade: the marked table's primary key never appears in the predicate, so
relationship tables and composite keys are marked with no special case.
No backfill, deliberately. is_marking_set_allowed coalesces NULL to '{}',
and the empty set is contained in every clearance, so every existing asset
stays visible the moment the column appears. Verified against a live
database. A marking can only ever REDUCE visibility, so adding the column
is inert until something writes one.
The index is on an expression, not the column, and this is not a style
choice: the rewritten predicate is COALESCE(marking_ids,'{}') <@
COALESCE(<clearance>,'{}'), whose left side is an EXPRESSION, so a plain
GIN(marking_ids) can never match it and would be pure write-amplification.
Verified on a 200k-row probe - with enable_seqscan=off the planner still
refused the plain index and accepted this one. Indexing the function itself
is impossible: it reads a GUC and is therefore STABLE, and only IMMUTABLE
expressions can be indexed. The index is not expected to be CHOSEN yet, and
that is correct: while nearly every row is unmarked the predicate matches
~100% of rows and a seq scan is cheaper. It is created now because doing it
later means an index build on a large live table.
Note that Asset.markingIds does not cause any filtering - it exists so the
set can be written and displayed. Nothing writes it through the API yet;
that is the next commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
findForIndexing grouped by a.asset_id and projected a.asset_hostname,
a.endpoint_platform, a.endpoint_is_eol, a.endpoint_arch and a.tenant_id
alongside it. That is legal only because Postgres knows a primary key
functionally determines every other column of its table.
Marking-activating assets breaks that. The scope inspector does not append
a WHERE - it REPLACES the table with an inline view:
assets a -> (SELECT * FROM assets a WHERE <scope>) AS a
An inline view has no primary key, so the functional dependency is gone
and Postgres rejects the query with "column a.asset_hostname must appear
in the GROUP BY clause". This single query accounted for the large
majority of the failures in the first honest full-suite run; DashboardApiTest
alone went from 19 failures to 25/25 green.
Two fixes exist and the right one depends on intent. Where GROUP BY pk is
a DEDUP DEVICE, DISTINCT expresses it without depending on the key
(commit 0c9a1f1). Here it is a GENUINE AGGREGATE - max(f.finding_updated_at)
- so the only correct fix is to list every non-aggregated projected column.
Behaviour is unchanged either way: grouping by the primary key plus columns
that are functionally dependent on it produces exactly the same groups.
This generalises to every future marking- or tenant-activated table and
belongs in the activation skill: the inline view is the mechanism, and any
GROUP BY that leans on primary-key functional dependency will break.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two unrelated shapes that a marking-active assets table can no longer
express, because every statement now goes through JSqlParser and the
fail-closed rewriter instead of straight to Postgres.
1. lower(concat('%', :name, '%')) is unparseable.
JSqlParser 5.2 cannot parse a function whose argument is a concatenation
containing a bind parameter. Hibernate renders JPQL concat as ||, so the
predicate reaches the parser as lower('%'||?||'%') and fails; the inspector
refuses what it cannot parse and the request 500s. /api/assets/options was
the visible symptom.
Bisected against JSqlParser directly rather than guessed:
lower('a'||'b') parses
n like concat('%',?,'%') parses
lower('%'||?||'%') FAILS
So the fix is to move lower() inside and let || stay the outermost
operator: concat('%', lower(:name), '%'). Same semantics, same plan,
parseable shape. Applied to the four repositories whose queries target
assets. Native queries are unaffected - they keep CONCAT(...) as literally
written and never become ||.
AssetGroupRepository and TeamRepository carry the identical pattern but
target tables that are not scope-active, so they still work. They are
landmines for the next activation and are called out in the skill rather
than changed here.
2. unnest(...) in a FROM clause is refused outright.
filterFromItem accepts a TableFunction only with an explicit LATERAL
prefix. unnest(e.asset_mac_addresses) inside a subquery FROM is implicitly
lateral and carries no prefix, so it is rejected as a shape the rewriter
has not reviewed.
Deliberately NOT fixed by widening the inspector. It is a fail-closed
security boundary, and loosening what it accepts as a side effect of an
unrelated repair is not a trade to make quietly. The query is rewritten to
array overlap instead, keeping the whole predicate in scalar functions:
e.asset_mac_addresses && string_to_array(LOWER(REPLACE(REPLACE(
array_to_string(:macAddresses, ','), ':', ''), '-', '')), ',')
Semantics verified against a live Postgres. The cardinality(...) > 0 guard
preserves the previous behaviour for an empty request: string_to_array('', ',')
yields {''}, which would otherwise match a stored empty string and make an
empty request match rows it never used to.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
) openaev-api/src/test/resources/application.properties SHADOWS src/main/resources/application.properties. Spring Boot loads the FIRST application.properties on the classpath and does not merge the two, so openaev.marking.active-tables=assets set in main was simply not in force for any test. The consequence was worse than a missing setting: the suite was green, and the greenness was reported as evidence that activation had no blast radius. It had one - 6 failures and 43 errors - but the dimension was off, so nothing could have caught them. A measurement of an inactive feature looks exactly like a measurement of a working one. Ordered deliberately after the two fix commits. Flipping this first would have left the tree red for two commits and made bisection useless; the fixes are correct queries independent of the dimension, so they are safe to land ahead of it. The comment in the file states the shadowing rule explicitly, because the next person activating a table will otherwise repeat this exactly: any table activated in main must be repeated here or it is not active in tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PUT /api/tenants/{t}/assets/{assetId}/markings - the write path for the
row side of the model. Until now nothing could label an asset through the
API, so the isolation tests had to seed marking_ids with raw JDBC.
On /api/assets rather than /api/endpoints on purpose: marking_ids lives on
the assets table, so one endpoint marks every asset category - endpoint,
security platform, AI target - instead of one endpoint per subtype each
repeating the same guard.
Replace-the-whole-set, like the sibling groups/{id}/markings, users and
roles endpoints. An empty list clears every marking and makes the asset
visible to everyone again.
Two guards, and they are not redundant:
MarkingEscalationValidator refuses any marking outside the caller's own
clearance. This is what makes self-lockout impossible BY CONSTRUCTION
rather than by care: the validator accepts exactly the markings that are a
subset of your clearance, and a row is visible exactly when its markings
are a subset of your clearance - the same set on both sides, so a
successful write can never hide the row from its own author.
Tenant isolation refuses a marking id from another tenant. This cannot be
left to the statement inspector alone: the inspector rewrites QUERIES, and
an entity already in the persistence context is served from Hibernate's
first-level cache without a query being issued at all. The escalation guard
covers that hole independently, since a clearance is per tenant and nobody
holds another tenant's marking.
An asset above the caller's clearance returns 404, not 403 - filtering
happens below authorization, so "you may not see it" and "it does not
exist" are indistinguishable by design, and saying 403 would leak the
existence of a row the caller must not know about.
Access control is the asset's own WRITE for now, matching the group
markings endpoint. The dedicated "assign marking" capability chain is
design Q8 and lands with Task 1; the TODO is in the code.
AssetMarkingsService is added to the TenantActiveTableAccessArchTest
allowlist with the reasoning recorded inline, the same shape already
accepted for TenantGroupService.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A Markings column on the endpoints list, rendering each marking as a MUI chip in its own colour. Without it the whole mechanism is invisible: an asset that vanishes from a list looks identical to one that was deleted, and there is no way to see WHY a row is or is not there. EndpointOutput gains asset_markings (ids only). Exposing them leaks nothing: a row only reaches you if its markings are already a subset of your clearance, so every id in the payload is one you were entitled to see by the time you could read the row at all. The mapper normalises a null column to an empty set rather than passing null through - unmarked and "no markings" are the same thing to every consumer, and the distinction is an artefact of the column being nullable so that activation needs no backfill. Ids are resolved client-side. Markings are not in the Redux store the way tags are, so ItemMarkings cannot follow the ItemTags/useHelper pattern; useMarkingDefinitions fetches the catalogue once per page and indexes it, which keeps a 50-row list at one request rather than fifty. ItemMarkings drops ids it cannot resolve instead of rendering a placeholder. There is no foreign key from assets.marking_ids to marking_definitions, so a dangling id is possible and must not blank the page. That defensive choice has a cost worth recording: a 403 on the definitions lookup and a genuinely unmarked asset look IDENTICAL on screen. Reading marking definitions currently requires ACCESS_TENANT_SETTINGS, which the Manager role does not hold, so today every non-admin sees an empty column even though the ids are in the payload. It is a display failure, not a leak. The fix is a dedicated ACCESS_MARKINGS capability mirroring ACCESS_TAGS - markings are reference data, and anyone who can see a marked row needs to read them - and it is recorded as a finding under step 5 of the implementation plan. api-types.d.ts is regenerated, not hand-edited. Widths in inlineStyles re-balanced so they still sum to 100. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ough (#7510) brainstorming/marking/demo/ replaces manual-testing-markings.md. A curl walkthrough in a markdown file rots silently and cannot be mutation-checked; these run. demo.sh asserts the full model end to end - 9 assertions, 9 green. It creates a throwaway user, group and asset per run, so it leaves nothing behind and can be run repeatedly. Mutation-checked, which is the only thing that makes the result meaningful. Restarted with --openaev.marking.active-tables= the run goes 8 passed, 1 failed. That single assertion, 3.2, is therefore the ONLY one that discriminates on its own; the header says so. The others are consistency checks that hold whether or not filtering is on, and reading them as proof would be the same mistake as the shadowed test property. Two ways a run can pass for the wrong reason, both hit while writing it: without RBAC every request 403s and the escalation assertion passes vacuously, so the demo group is given the Manager role; with BYPASS isAdminOrBypass skips marking filtering entirely, so it deliberately is not. DEMO=1 stops at 7 UI checkpoints, one per step, each placed AFTER that step's assertions so the screen and the database agree. The two that carry the idea are the asset disappearing from a list while remaining visible in the admin window, and reappearing on a grant write with no TTL to wait out. group-markings.sh and mark-asset.sh are the two sides of the containment test - what a group GRANTS versus what a row CARRIES - and each header points at the other, because conflating them is the easy mistake. Both resolve human names to ids, so no UUIDs are pasted, and an unknown name prints the nine that exist (there is no TLP:ORANGE). Shared plumbing is in _common.sh. demo.sh deliberately does not use it: a demo handed to someone else should be one file readable top to bottom, and that is worth more than avoiding the duplication. The bracket form must be quoted - [TLP:RED] is a glob, and in zsh it fails with "no matches found" or, if a single-character file exists in the directory, silently expands to THAT instead. The plain space-separated form is listed first for this reason. Also records under step 5 the capability gap the demo exposed: reading marking definitions requires ACCESS_TENANT_SETTINGS, so the Markings column is empty for every non-admin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ruth (#7510) user-stories.md reads like a specification, so it gets treated as one. It is not: it is a point-in-time export of Notion tasks 590, 591 and 592, kept next to the technical design so those documents can quote acceptance criteria without sending the reader to another tool. The banner says so at the top, links the EPIC, and lists the three task IDs so any statement can be traced back to the thing that actually governs it. The drift is not hypothetical. Task 2 already appears TWICE in this file, and the two copies are not identical - the second carries an "Important Flags" section the first lacks. Nobody noticed, because a stale export looks exactly like a current one. That example is cited in the banner rather than described in the abstract, so the warning is evidence rather than boilerplate. The failure mode worth preventing is someone editing acceptance criteria here: the change would be invisible to PM and stakeholders, would not be reviewed, and would be silently destroyed by the next export. The banner states the resolution rule explicitly - if this file and Notion disagree, Notion wins and the difference is a bug in this file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
📖 Documentation check — ✅ Skipped
|
Contributor
|
✅ Container vulnerability scan — Passed Previously reported findings are no longer present.
View workflow run · Standard JSON report · UBI9 JSON report Updated from CI run attempt 1. |
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.
Proposed changes
Testing Instructions
Related issues
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...