feat(publish): personal cloud drive as an opt-in publish destination - #7199
Conversation
github-actions
Bot
added
the
readiness: checking
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @chenmingwei23 overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: action required
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of The description's claims check out against the diff — the seam changes, the delete-refusal semantics, and the opt-in registration are all as stated. I have enough to render the design review. Design-Verdict: CONCERNS Sound, honestly-scoped design; the acknowledged gaps compose into a real user trap that ships live, not deferred. Watch
[DESIGN-REVIEWED] 362153a |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is gathered: patch read in full, intent read, mechanisms and counts verified in the repo ( First-Principles-Verdict: CONCERNS
What this change shipsIntent: give a stock build a working publish destination (one pooled S3+CloudFront drive) and make withdrawal trustworthy — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 362153a |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBlocking FINDING — src/kiro_crew/publish_sync.py:833 — [OPUS-REVIEWED] 362153a Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
4a2f293 to
5dc12d3
Compare
github-actions
Bot
added
readiness: checking
5dc12d3 to
6211b22
Compare
github-actions
Bot
added
readiness: checking
6211b22 to
62d7ff2
Compare
github-actions
Bot
added
readiness: checking
62d7ff2 to
08b614a
Compare
github-actions
Bot
added
readiness: checking
08b614a to
0fdfc46
Compare
|
Note on the second finding at The finding is correct and I cannot act on it from this branch. Two things follow, and they point in opposite directions, so I am stating both rather than picking the convenient one:
Writing the row requires rebasing onto |
|
|
|
|
|
There is no advisory and no package. The log names no GHSA, no severity and no dependency. Both PRs fail on one line, byte-identical:
So the gate did not find a vulnerability. Neither PR introduced it, and neither PR's own dependencies are involved. #7199 touches zero lines of
The lane passed on this PR at 15:09:22Z with 1028 packages, and has failed on every run after Classification: main-owned, and a tooling budget rather than a dependency. Not a new advisory on a main dependency either — that was the natural hypothesis and the log rules it out. Two consequences worth being explicit about, because both differ from the obvious action:
What actually fixes it has to land on I have not changed anything in #7199 for this and will not. |
|
|
|
|
Note on the second finding at Unchanged and still awaiting a maintainer's sequencing decision. It has been drawn on four consecutive heads and answered each time — most fully in comment 5527686878, with the design sketch on #8148. The short version, so this note stands alone: a handle pins the profile name, not the account behind it. Repointing that name at another account and then deleting resolves the name to the new account, removes nothing, reports success, and clears the record — leaving the original copy public with its only handle erased. Every claim that said the account was bound was corrected in Binding the account id is not improvised here because it changes what a handle is: the separator is partitioned on its first occurrence so a profile name may contain it, which makes appending a field ambiguous, and six parse sites read the current shape. I have flagged this one as weighing against shipping even opt-in, because unlike the race above it needs no unusual timing. That remains my recommendation and it is not mine to decide. |
|
@bolichen97 — you were right, and one part of your review was still unsatisfied until just now. Working through your four required changes against the current head Done earlier:
Not done until now, and worse than the version you flagged. Your first item asked me to rewrite the section 3 delete paragraph and both section 4 "Delete path" bullets to state the actual split. I updated them to the split as it stood when you wrote — unreachable proceeds, reachable-rejection aborts — and then a later revision withdrew that split entirely: an unreachable destination now refuses too, because "cannot be reached" is not evidence of "is not being served". Expiring credentials and a deleted bucket look identical from here, and the copy may still be world-readable. I never revisited the body after that change, so it kept describing behaviour that had been deliberately reversed. Concretely, three passages were wrong, and two of them named tests that do not exist:
All three are corrected, and I added a check I should have been running from the start: every Your framing was the accurate one and I want to acknowledge it specifically. You wrote that a reviewer reading sections 3 and 4 approves on the understanding that a delete can never be blocked by a remote system, and that the spec half ships to readers who never see the thread. That is exactly what happened again in the other direction: the body promised a weaker guarantee than the code provides, naming evidence that had been inverted. The behaviour has only ever moved toward refusing more, so nobody was told a delete was safer than it is — but a reviewer was being handed a description that no longer matched either the code or its tests. Nothing about the behaviour changed in this update; it is body-only. The delete path's actual contract, unchanged since the withdrawal ruling: refuse unless there is nothing left to withdraw or the destination confirmed the withdrawal, and there is no second way out, since unpublishing keeps the record too unless a removal or a confirmed absence releases it. Two open items are recorded and awaiting a maintainer decision rather than more work from me — the handle pinning a profile name rather than an AWS account (#8148), which I have flagged as weighing against shipping even opt-in, and the publish/delete window on the same issue. The |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Correcting my own note above, because following it would leave people stuck: the fix has landed on
Why my PR is still red, and why re-running cannot fix it. I re-ran the audit on #7199 at 11:30Z, five hours after that commit landed. It failed with the old message,
Practical consequence, which reverses half of what I wrote earlier: I said neither a re-run nor a rebase clears this. The re-run half is now doubly confirmed and I can name the mechanism. The other half was wrong — any new push clears it, because that recomputes the merge ref against So the remaining red on a PR that has not pushed since ~04:34Z is a stale artifact of a workflow that no longer exists in this form, not a live signal. Nobody needs to re-run anything; the next push retires it. I am leaving #7199's red in place rather than pushing a no-op commit to refresh CI. Thanks for the retry-and-budget work — worth noting your first suggestion turned out to be the right one for the electron-sized case, and the release-only move handles the large lockfile that a retry could not. |
|
@bolichen97 — re-requesting your review, because the change you reviewed is not the change that is here now. Laying out what moved so you can judge it rather than take my word for it. 1. The PR no longer makes this drive the default destinationIt reviewed as "personal cloud drive as the default publish destination". It is now opt-in: the provider registers under its own registry key instead of the seam's default key, so a publish that names no destination resolves the default key, finds it unregistered, and returns the same This changes reachability, not correctness, and I do not want to overstate it. Three findings on this PR are still live on the opt-in path — they are not fixed by the downgrade:
All three are lifted into #8148 as one publication-existence contract, because they interlock: reserving a record before upload needs the absence oracle to clean up a record whose object never appeared. Opt-in narrows who can reach them; it does not close them. 2. Your finding, and a second instance of the same classYour four required changes are done, detailed in my earlier reply. The part worth repeating here is that the first one had been re-falsified in the interim: I rewrote the body to the split as it stood when you reviewed (unreachable proceeds, reachable-rejection aborts), and a later revision withdrew that split entirely — an unreachable destination now refuses too, since "cannot be reached" is not evidence of "is not being served". Three passages still described the old behaviour and two of them cited tests that no longer existed under those names. Fixed, and I now mechanically verify that every test name cited in the description resolves in Separately, and not something you raised: the same class of defect turned up in the acknowledgment copy, which told the user they could make the artifact private or unpublish it later — controls this destination does not offer. It now says the copy stays public until it is withdrawn. I mention it because it is the same failure mode you named, found by looking for more of it. 3. Automated validation, as facts rather than a conclusion
4. What I am askingPlease re-read the opt-in version. If your finding is addressed, clearing the |
|
|
Note on the other findings on this head. Not a disposition record — the boot-path record above claims this lane's span, and one span takes one record.
The rule is real and it is new:
Both of the last two were raised and answered on earlier heads; they are re-drawn here because the rebase produced a new head, not because anything changed. |
|
|
Note on the two remaining findings. Not a disposition record — the ErrorNotice record above claims this lane's span.
You are right about the mechanic: an
Requiring
Your chain holds. The Your fix is the right shape and it needs a capability the store does not have: "clear this publication and delete only this generation" means |
Status summaryPosting a state snapshot rather than a change, so this is scannable without reading the thread. What it delivers. The public edition registers no publish destination, so publishing an artifact returns 503 out of the box. This registers one: a private S3 bucket plus one CloudFront distribution in the operator's own AWS account. It registers under its own key rather than the seam's default, so it is opt-in — the picker lists it and a caller can name it, but a publish that names no destination still resolves the unregistered default key and still gets the same 503. Shipping it as the default needs the contract below; that is why it is not the default here. Three findings are lifted out, not fixed. They are one question wearing three faces — what proves a published copy exists, or no longer exists, and who may act on that — so answering them separately means answering the same question three times with three different rules. They are tracked together as one publication-existence contract in #8148:
All three need overlapping operations to reach. A fourth item of the same family did not — a handle pinned the AWS profile name, so repointing that name and deleting stranded a public copy on the first try, with no concurrency — and that one is fixed here rather than deferred: the handle now pins the account id and every mutation path verifies it before acting. Real defects this found and closed, which is the part worth weighing against the diff size:
Automated validation. On the last fully settled revision ( One fact about that red, offered as a fact rather than a conclusion: this lane is not deterministic on this PR. It printed no findings on a revision carrying a defect it had itself marked BLOCKING one revision earlier, where the only intervening change was docstring prose; it then drew two findings on code identical to that clean run after a description edit re-triggered it; and it has re-run and flipped its verdict on an already-settled revision. Every finding it has raised has a written disposition in this thread. Whether that makes its current verdict actionable is a reviewer's call, not mine — I have not re-run it to chase a greener sample, because a re-run can also destroy a real finding. Size is the honest obstacle: 41 files, +8120/-105. It is one commit and the history is linear if that helps to read it. |
|
Addendum on the two Both failed inside They are also not deterministic, which is worth stating because it decides whether a rerun is honest or wishful. Two runs of the same job on the same commit failed on different sets: I re-ran the failed jobs once on that basis. Current state: 63 lanes, 56 green, 1 queued, 2 red — Windows shard 4 above, and I have deliberately not pushed anything to refresh CI. A push recomputes the merge ref and re-rolls all 63 lanes, discarding green results to re-sample them, which trades a real signal for a fresh gamble. Nothing here is a code change. |
The publish seam shipped with no destination registered in the public
edition, so publishing an artifact was unavailable out of the box. Register
a concrete destination -- a private bucket plus one CloudFront distribution
in the operator's own account -- through the existing platform registry.
One pooled distribution serves every published artifact. A distribution per
artifact pays a cold create on each first publish (the URL does not resolve
until it propagates), consumes an account distribution quota slot per
artifact, and presents a new distribution over a still-empty origin, a shape
scanners flag as a dangling origin and may auto-disable.
The drive carries its own tag rather than the deploy engine's site tag. The
engine groups sites by that tag and skips resources without it, so tagging
the pooled drive as a site would list it in the deploy surface as an
ordinary one-off site, where a recall empties it and a destroy deletes it --
one action on an unfamiliar row would wipe every artifact ever published
here. Discovery, with its ownership and ambiguity hardenings, is implemented
against the drive's own tag instead of inherited. The engine hard-codes its
site tag on the create call, so the drive's tag is added and the site tag
removed afterwards; every later publish re-attempts that removal, which is
what keeps a failed attempt from leaving the drive listed as a site forever.
A publication's account identity is bound into its external id at publish
time. The seam persists a provider registry key and re-resolves it later, so
a key meaning "whatever account is default right now" would make a later
unpublish run against a different account -- reporting success while the
original object stayed served. The public URL is built from the id's random
half only, so an account label never appears in a link.
There is one registry key, the seam's default. Per-account keys were tried
and withdrawn: a provider id is validated against ^[a-z0-9-]{1,32}$ at the
HTTP boundary, so an account-qualified key renders as a picker row that
rejects its own submission, and list_providers does not dedupe, so a second
key for one provider renders twice. Choosing an account per publish needs a
real publish argument on the shared seam and is left to that change.
Removal and take-private are never gated on the delivery network being
healthy. The orchestration clears the local record whether or not the
destination delete happened, so refusing there would report a withdrawal
that never occurred while the content stayed public. Publish and make-public
do assert it, because those hand out a link.
Withdrawal is no longer best-effort at the seam either. The publication block
is the only handle to content that may still be served, so a failed removal
keeps it and raises instead of clearing it and stranding the remote copy;
deleting the artifact remains the escape hatch for a destination that is gone
for good. A visibility change that turns an artifact public also fills in a
link the private publish could not derive, additively, so a destination that
returned its own URL keeps exactly that URL.
Every step that can leave the remote and the record disagreeing is ordered
against which failure is worse. A head-object failure is no longer read as
"object already gone": only a confirmed not-found is an absence. On the way
out to public the source delete and the cache purge are cleanup, because the
object is already served and raising would record it as private; taking
something private both are load-bearing and a failure is reported. When both
prefixes hold a copy the destination wins and is never overwritten from the
stale source, which may predate a push.
Pushes honour the concurrency token the provider declares: the content digest
is stored as object metadata and compared before writing, a remote change
made out of band returns a conflict instead of being overwritten, and a
MISSING digest counts as a conflict too, since every object this module
writes carries the metadata. A push types its bytes from the file it was
handed rather than the remote's old header, so an artifact re-saved under a
different kind is not served under the previous type.
Shared folders are not implemented. The provider declares the SHARING
capability, which means it can change an artifact's visibility -- not that it
has a per-principal grant list. sharing_model declares supports_shared=False
so the UI never offers one, and a request carrying recipients is refused
explicitly rather than silently downgraded to public.
A same-named response-headers policy is verified before it is reused. The policy
is account-global and found by a fixed name, so a name match alone was enough to
attach it -- and this product runs agents holding the operator's own AWS
credentials, so a policy of that name can exist without the sandbox CSP. Reusing
it would hand every published document a real browser origin, which is the one
cost of pooling the module refuses to pay. A name match now reuses only when the
policy's CSP equals the sandbox directive exactly with Override set; anything
else raises and names the remedy, because the name cannot be reused and cannot
be recreated automatically. Independently of any attacker, never comparing the
config also meant a later change to the directive would never reach an account
that already held the policy. The directive is spelled once and read by both the
create and the verify path, so the two cannot drift; the list call already
returns each policy's full config, so verification adds no request.
A publish that succeeds but cannot serve its link yet is no longer reported as a
failure. The rollout notice was stored in the publication's last_error, which
every consumer reads as failure: the first publish to a fresh drive rendered red,
the link was withheld from the success branch that renders it, and a danger
banner persisted on a healthy artifact until an unrelated push cleared the field.
The notice now has its own field, empty by default so existing records load
unchanged, and it is cleared wherever a later success settles the question. The
link renders with a neutral line beside it instead.
Two states in the publish panel that could only end in failure are closed. An
unconfigured core destination offered a live Publish button, so a stock install
could read the remedy, publish anyway, pass the exposure acknowledgment and only
then fail; it now shows the remedy and a way to set the destination up, and no
Publish. The empty state was not gated on either provider query, so a stock build
announced that no destination existed before the core one arrived -- the exact
false message this change exists to remove.
Discovery that cannot see no longer reports absence. Enumerating the account's
distributions, or reading one distribution's tags, could fail on a throttle and
be swallowed as "nothing found" -- and the caller does not refuse on nothing
found, it creates: a second distribution over the existing bucket with the
bucket policy repointed at it, so every link already handed out answers 403,
and a second tagged distribution that wedges each later discovery on the
ambiguity refusal. A transient error became permanent breakage. Both sites now
raise and say nothing was changed, so the next attempt simply succeeds. A
genuinely empty result still means absence and still creates. Withdrawal is not
affected: that path never creates, so raising there is the same retry it is
everywhere else.
Deleting an artifact whose withdrawal failed now depends on whether a retry
could work. The publication is the only handle able to withdraw a copy that may
still be served, so discarding it on any failure strands that copy with nothing
able to reach it. Refusing on any failure is not the answer either: the strict
withdrawal path tells the user to delete the artifact when a destination cannot
be restored, so refusing there would leave an artifact that can never be
deleted and turn that instruction into a lie. A destination that cannot be
reached at all is that gone-for-good case and the delete proceeds; a
destination that answers and then rejects the removal is retryable, so the
delete stops and keeps the record. The withdrawal reports which of those
happened rather than swallowing everything.
A publish notice now says which state it is reporting, because the interface was
asserting one it could not know. A notice was free text and both render sites
threw it away to print a single fixed line promising the link would work in a few
minutes. That is true while a distribution propagates and false when one is
disabled, where the link never resolves until a person re-enables it -- so the
copy sent the user away to wait for something that was not going to happen, and
the only action that fixes it was never offered. The provider now returns a code
beside the text, the record keeps it, and each case gets its own wording;
anything the interface does not recognise falls back to a line that promises no
time at all.
A stale notice can now be cleared, because nothing ever cleared it on the
ordinary path. It was recorded once at publish time and reset only by a push, a
pull, an overwrite or a visibility change, so a link that had finished rolling
out kept its warning on every later visit. The notice is re-checked against the
destination on request and cleared only when the condition really has cleared --
asked for rather than polled, since the answer costs a call to the destination
and a timer would either clear it without looking or ask on every visit.
The consent copy names a way out that exists. It promised the reader could make
the artifact private or unpublish it; both of those calls are defined and reached
from nowhere in the interface, so it named two controls a user cannot press. It
now names deleting the artifact, which they can.
A half-present drive is no longer read as no drive. A tagged distribution whose
bucket is missing was reported as absence, and absence is what sends the create
path into partial-create recovery: a second tagged distribution over a fresh
bucket, and discovery wedged on the ambiguity refusal from then on. It now raises,
naming what was found and what is missing; a drive that is genuinely absent still
creates. Nothing tries to repair the half-present case, because which half is
authoritative is not something this module can know.
The drive answers the re-probe itself, because the seam's default answer is "I
cannot look" and that would have made the whole reconcile a no-op for the one
destination that produces these notices: the control would call it and the stale
warning would never clear. The provider re-resolves its own drive and re-derives
the notice, and every way that can fail answers "could not establish" rather than
"cleared" -- inferring the latter from a failed look would drop a warning that is
still true.
Reachability is asked about the publication rather than the destination, because a
publication is bound to one account and the withdrawal paths were asking whether
ANY account was configured. With two registered and the bound one removed the wide
answer stayed true, so a withdrawal was attempted against an account that no longer
existed, and the resulting failure was classified as a rejection a retry could fix.
The artifact could then be neither withdrawn nor deleted: both paths advised
retrying something permanent. The paths acting on an existing publication now ask
about that publication's own account. Providers binding nothing per publication
inherit the destination-wide answer, so only the drive implements it.
This enlarges what counts as unreachable, which is the population the delete path's
escape hatch applies to -- a removed account now takes the hatch instead of
refusing. That is the correct reading of a permanently absent account under either
resolution of the open question about whether the hatch should exist at all, but it
does mean the decision covers this case too.
The re-probe's AWS calls run in a worker thread. Every call in that body is a
blocking CLI subprocess, and the override ran them straight on the coroutine while
every sibling method wraps the same work in a thread, so one re-probe froze the
single gateway event loop across several serial subprocesses -- stalling every
task, the liveness heartbeat included, until the watchdog fired. The repo's
no-blocking-call ratchet cannot see it, because the subprocess is four frames down
behind the drive lookup, so the test asserts the property directly by recording
which thread the AWS layer is entered on.
A time-boxed exposure is no longer promised for a link that cannot expire. The
chosen expiry lives in one piece of state shared by every destination row, and a
row that cannot expire only HID the control -- leaving the choice standing
underneath it, so picking 72 hours on a row that can expire and then switching to
one that cannot left the acknowledgment promising a window that would never close.
The value now derives from the same predicate that hides the control, rather than
being reset when the selection changes, so the two cannot disagree: no missed reset
or render ordering can bring the promise back. Asserted on the derivation itself,
because the control is a Radix trigger rather than a native select and a
render-driven test of this passes whether the bug is there or not.
An artifact whose bytes are not in its text content is refused rather than
published empty. A kind="image" artifact keeps its raster beside the artifact and
carries no text body, and the publish pipeline is str-typed end to end: the kind
map has no entry for it, the fallback types it text/plain, and the destination
received a ZERO-BYTE object which was then recorded as a successful publish and
handed back as a working link. Nothing downstream could tell that from a real
publish. Serving images is not the obstacle -- the drive is a blob store and
declares every kind native -- so real support is a bytes path through the render
and tempfile steps, which is a larger change than this refusal.
The notice re-probe keeps its documented promise when no provider is registered.
It states that an unavailable provider leaves the stored notice untouched, but
provider resolution ran before that check and raised, which the handler turned into
a 503 on a read-only reconcile the caller can do nothing about. Resolution failing
is the strongest form of unavailable, so it now takes the same path.
A provider's notice text is scrubbed where it is first recorded. Every other
provider-derived string that reaches the dashboard passes through a redaction step
on the way -- the handlers scrub provider exception text before it reaches either
the UI or the audit log -- but a notice is persisted into the publication and
serialized straight out, so it reached neither. The re-probe path added later did
redact, and its comment claimed parity with the module's other provider-string
sinks; there were none, and the publish path where a notice is FIRST recorded was
the gap. For this drive it changes nothing, because the text is this module's own,
but notice is a seam field any provider fills, so the guard belongs at the sink
rather than at the one implementation that currently happens to be trustworthy.
The concurrent-creation path reasserts the read policy like the other reuse paths.
That policy pins the distribution ARN, so it cannot be installed until the
distribution exists -- and since the distribution is born carrying the drive's tag,
it cannot be installed until the drive is already discoverable. A first publish that
lost PutBucketPolicy to a denial or a throttle therefore leaves a drive that the
in-lock recheck finds complete, and returning there without re-installing handed
back a link to a bucket the delivery network cannot read: 403 for the life of the
account, reported as a successful publish. The two other reuse paths already
re-installed; this was the one that assumed the call had succeeded.
The consent copy no longer promises the exposure ends at delete. It said the link
stays public until you delete the artifact, which is false exactly when the
destination cannot be reached: that delete proceeds locally and leaves the copy
served. It now says the exposure ends when the published copy is withdrawn and names
deleting the artifact as what requests that withdrawal -- true whichever way the
open question about the unreachable-destination path is settled, so the copy stops
depending on that answer.
The racing-creation branch fails closed for the caller that hands out a link. Its
read-policy guard was wired to require_serving, but a publish resolves the drive
with that flag false -- it uploads before it checks the rollout -- while still being
the one path that returns a URL. The helper's own contract says the question is
deliberately not require_serving for exactly that reason, so the branch warned
instead of raising, uploaded, and handed back a link to a bucket the delivery
network cannot read. link_promised now threads from the caller through to it, and a
private publish still proceeds, because refusing there would turn an optional
permission into one every withdrawal requires.
A withdrawal that outlives its artifact no longer deletes the replacement. The
destination call is a network round trip and the local delete removes by slug, so a
delete and recreate landing in that window left the delayed request removing an
artifact the user never asked to remove and that nothing restores. The ordering note
argued a save in that window is included in the delete that was asked for, which
holds for a save to the same artifact; a recreate is a different artifact wearing
the same name. The generation is re-read after the call and a mismatch aborts.
A push no longer clears the serving notice. It writes bytes to the object store,
while the notice describes whether the delivery network has finished rolling out or
is switched off -- independent conditions, since puts succeed normally against a
distribution that is still deploying. Treating a successful upload as evidence about
the network hid a true warning while the link still did not resolve. Two tests
asserted the old behaviour and one stated the premise outright; both now pin that
only a re-probe, which asks the destination, may clear it.
The delete path's generation re-read runs off the event loop. Reading an artifact
returns it WITH content, so on a large one it is a multi-megabyte synchronous read,
and the handler is async -- inline it stalls the gateway for every other task. It
now goes through the module's own off-loop helper, which propagates the exception
unchanged so the surrounding guard is untouched. The capture read at the top of the
same handler has the same shape and is left alone: it predates this change and is
someone else's defect to weigh.
The drive stops advertising a kind the seam will refuse. It answered NATIVE for
everything, which is true of the destination -- a blob store can hold any bytes --
and false about what a publish does, because the render path is text-typed and an
image keeps its raster outside the artifact's content. The share-panel picker only
offers a provider whose answer is not unsupported, so an image artifact was offered
a publish that then refused it. The set of kinds the seam cannot carry now lives on
the provider interface both sides already import, so the declaration and the refusal
read one source and cannot drift.
All three publish-outcome readers carry the notice discriminator. Two of them copied
the machine code alongside the human notice; the app-provider confirm branch copied
only the notice, and the copy selector keys off the code -- so a disabled-distribution
notice rendered the generic warning instead of the remedy that tells the user how to
re-enable it, which is the one case whose whole purpose is that instruction. The pin
is a source-level one rather than a rendered-flow assertion, because the defect is a
field missing at one of several sites: a test that drives a single path would have
stayed green on the other two, which is how this survived being introduced.
The serving re-check resolves the drive read-only. It went through the resolver that
re-asserts the bucket policy whenever the distribution carries an ARN, which is a
policy WRITE -- so a control whose whole purpose is to look again could mutate the
account, and it is reachable from a state where publishing was refused, letting a
caller who could not publish drive that write through the notice path. It now takes
the read-only lookup. The resolver's own comment claims the only caller asserting the
network is the make-public path; that claim was true until this re-check started
sharing it, and taking the read-only lookup keeps it true rather than widening it.
A docstring also still listed push among the operations that reset the stored notice.
An earlier change in this branch made the push path preserve both notice fields, so
that list described behaviour the code no longer has.
The drive's headers policy now denies framing. It set a content policy, content-type
options, strict transport and a referrer policy, and said nothing about who may FRAME a
published document -- so any third-party origin could frame a public artifact and drive
clickjacking against whatever it renders.
Both mechanisms are set, and the placement is the point. The directive rides in the
content-policy constant the reuse path already VERIFIES: a same-named policy whose
content policy does not match exactly fails closed, so framing protection cannot be
skipped on an account that already holds a policy under that name. Expressed only as
the legacy header it would have been reused unverified, which is the same fail-open the
content-policy comparison exists to prevent. The legacy header is set as well, for
browsers that honour only it, and both deny rather than allow same-origin framing: one
drive serves every artifact from a single domain, so "same origin" here is another
published document rather than a trusted host -- exactly the cross-document relationship
the opaque origin exists to deny.
Both overwrite branches preserve the serving notice too. An overwrite force-pushes
local bytes over the remote's current version, which is an object-store operation; the
notice describes the delivery network -- still rolling out, or switched off -- and the
overwrite probes neither. Blanking it there asserted that the link resolves on the
strength of an operation that never asked, the same false premise the push path had.
The error field still clears, because that one does describe the operation that just
ran. Only the re-check, which asks the destination, may clear the notice.
The withdrawal-handle contract, in one rule: an artifact's local delete proceeds only
when there is nothing left to withdraw, or the destination confirmed the withdrawal.
Everything else refuses, and says why. Refusing a delete is recoverable -- retry it, or
unpublish and accept the exposure in the open. A world-readable copy whose only handle
was just erased is not.
A destination this process cannot reach no longer counts as gone. Unreachable describes
our access, not the object, which may still be served to everyone; the delete used to
proceed there and drop the record, which is precisely how a copy ends up with no handle.
The one outcome that still permits the delete is a CONFIRMED absent destination, and the
provider signals that with a dedicated exception type rather than a phrase in an error
message -- a throttled or unauthorized reply carries the same words while the object is
still live, so reading the text would hand back "gone" for a copy that is not. This
widens the set of deletes that refuse, which is the intended trade rather than a
side effect.
First publishes of one artifact are serialized in the engine. Two concurrent ones each
minted their own destination id, uploaded a distinct public object, and the record
written second replaced the first -- one copy served with its handle recorded nowhere.
The engine holds the identity that makes this fixable: a provider is handed a rendered
file and no artifact id, and mints the id itself, so its own per-id locking cannot see
the collision. Serializing per artifact one layer up fixes every provider at once and
leaves the provider interface untouched. The lock spans the whole operation because the
choice between first-publish and re-publish is itself what was racing.
The folder cascade withdraws what it destroys. Cascading a folder deletes artifacts
through a path that knows nothing about publications, so it erased records while the
copies stayed served -- the same defect through a different door. It now withdraws every
published copy in the subtree first and destroys nothing until they are all withdrawn;
the first copy that will not come down refuses the whole cascade and leaves the folder,
the artifacts and their handles in place.
Deliberately not included: a persisted ledger of orphaned copies. That is the shared
after-the-fact remedy for all three of these, and it is a feature with its own storage
and lifecycle rather than a patch belonging to this change.
The cascade's withdrawal preflight is not on its own enough to let it destroy, because
nothing holds a lock across the preflight and the destruction that follows -- the folder
tree and the artifact store have independent locks and taking both invites an ordering
deadlock. An artifact filed into the subtree after the preflight enumerated it reaches
the destruction still holding a publication nobody withdrew, and destroying it erases
the only handle able to take that copy down. A comment on that path documented the race
as accepted, which it was while the consequence was a stale folder id readers already
degrade to Unfiled; making the cascade withdraw is what turned the consequence into
destruction, so the comment is now corrected rather than left to mislead.
The invariant is therefore enforced where the destruction happens instead of by trusting
the earlier scan: the cascade refuses to destroy any artifact that still has a
publication, keeps it, and reports it in the summary and to the user. The preflight
clears the record of every copy it withdrew, so a still-published artifact at that point
is exactly a late arrival. The folder-tree change is already committed and cannot be
rolled back there, so the artifact survives unfiled -- recoverable, which an orphaned
public copy is not. The narrower window inside that scan, a publish landing between the
scan and that artifact's own delete, is disclosed rather than closed: closing it means
moving the invariant into the store's own delete, which every caller would inherit.
The cascade asks the delete itself to refuse rather than checking first. A check in the
cascade loop reads a snapshot, so a publish landing between the scan and that artifact's
own delete was still destroyed -- the same defect one window narrower. `ArtifactStore.
delete` now takes an opt-in `refuse_if_published` and re-reads the record inside the
same lock as the removal, which is what makes the answer hold. The flag defaults to
False so the single-artifact path is untouched: it is answered at its handler, which
attempts the withdrawal and refuses on the outcome. A refused artifact is reported, not
skipped silently, because a partly-refused cascade must not read as a completed one.
Unpublishing a confirmed-gone destination now completes instead of looping forever.
`reachable_for` resolves the PROFILE, not the drive, so a drive deleted under a
still-registered profile passes the reachability guard and then raises inside the
provider -- where a broad handler reported it as a removal the destination "did not
confirm", advising a retry that can never succeed. Combined with the delete path
refusing on that same destination, the artifact could be neither unpublished nor
deleted. A typed DriveNotFound now releases the record, because there is nothing left to
withdraw; the type is what carries it, never the message text, for the same reason as on
the delete path.
The unreachable-destination refusal no longer offers deleting the artifact as the way
out. It told the user "if it is gone, delete the artifact to drop the record along with
it", which was true while that delete dropped the record and false once it started
refusing on an unwithdrawn copy. Naming a remedy that is guaranteed to refuse is worse
than naming none, so it now points at the one action that can work.
A destination lookup that finds nothing is no longer reported as confirmed absence. The
drive resolves itself by TAG, so a miss says the LOOKUP failed -- a tag removed by hand
or a transient answer from the tagging API both leave the bucket and distribution
serving. Reporting that as proof let the withdrawal paths release the local record, the
only handle able to take that copy down, on evidence no stronger than the substring match
the typed signal was introduced to replace. The type was right; its single raise site was
asserting something it could not know.
The lookup miss is now a plain refusal, and no site raises the confirmed-absence type at
all -- the two branches that read it are unreachable and say so at the definition. Proving
absence needs a positive probe that asks the destination about the resource the
publication names, rather than asking a directory whether it can still find it. Until
that exists, nothing releases a handle except a confirmed withdrawal.
The cost is stated rather than hidden: a drive genuinely deleted under a still-registered
profile can no longer have its record cleared, because reachability resolves the profile
and not the drive. That is the deliberate side of the trade. A record that will not clear
is recoverable by its owner; a world-readable copy whose handle was erased is not.
The drive registers under its own key rather than the seam's default one, which is
what makes it opt-in. An unnamed publish resolves the default key, that key stays
unregistered, and such a publish gets the same 503 as an edition with no provider at
all -- while the picker still lists the destination and a caller can name it. The
feature ships; it just does not capture the path nobody chose.
Holding the default back is a scope decision, not a gap in the transfer. Five review
rounds converged on one question rather than five: what proves a publication exists,
or no longer exists, and which paths may act on that. Answering it needs agreement
across the publish engine, the artifact store and the folder store -- three lock
domains that do not nest, whose cross-lock coordination this change set already
declined once for ordering deadlock -- plus a positive absence probe, because
resolving the destination by tag can only report that a lookup failed and never that
a resource is gone. That contract is #8148.
Two windows depend on it and are left open rather than patched a sixth time: a first
publish can race a concurrent delete around the engine's per-artifact publish lock,
which the delete paths do not take, and an unpublish against a drive deleted under a
still-registered profile refuses instead of releasing the record. Opt-in is what makes
shipping them acceptable: each is now reachable only by someone who chose this
destination deliberately, rather than by every user on the default path. Moving the key
to the default is the last step of #8148, not a step of this change.
An earlier round declined this same reduction on the grounds that it removed the
change's purpose, which was right on the information available then. What changed is
knowing that shipping it as the default requires the cross-store contract, so it is no
longer the same trade.
Two spec claims are corrected to match the registration. The platform-context walkthrough
still said the default registry registers the drive "under DEFAULT_PROVIDER, so publishing
is available out of the box", which the opt-in key makes false in both halves. And the
artifacts endpoint table never listed the notice re-check endpoint this change adds, so the
structural map was missing a route that exists -- the row now also records that the
re-check resolves through the destination's READ path, which is the property that keeps a
re-check from provisioning anything.
An unreadable delivery-network flag no longer reads as "checked, and the condition
cleared". The notice derivation asks CloudFront whether the distribution is enabled, and
a failed read of that one flag was reported as "enabled" -- so a throttled or
unauthorized answer walked past the disabled branch and reached the final empty pair,
which the re-probe takes as permission to clear the stored notice. That dropped a warning
that may still be true, and the method's own contract two functions away says every
failure must instead report that nothing could be established.
The flag now has three answers -- enabled, disabled, and could-not-read -- because its
two callers need opposite handling of the third. The re-probe reports could-not-read as
"nothing established" and leaves the record untouched. The publish path keeps treating it
as no objection, deliberately: the shipped IAM tier does not grant
cloudfront:GetDistributionConfig, so refusing there would turn a permissions gap into a
refused publish, which an existing test pins. Same value, opposite handling, because the
cost of guessing is reversed between the two.
Unpublish's contract is also corrected. It still described itself as the deliberate way
out -- the owner accepting that the published copy may remain -- which stopped being true
once it began keeping the record unless a removal or a confirmed absence says otherwise.
It now states what it does, and that no exit currently exists: the confirmed-absence
branch is unreachable because nothing raises that type, so a publication whose
destination stopped answering cannot be cleared from here and deleting the artifact does
not clear it either. Giving an owner a real way out needs the absence probe tracked with
the publication-existence contract, not a looser rule in this function.
The claim that an owner has a way out is removed everywhere it was still stated. Last
round corrected it in unpublish's own docstring; a grep for the claim rather than the
symbol found it standing in four more places -- the delete-outcome enum's rule paragraph,
the delete handler's comment explaining why it refuses, and two passages in the artifacts
spec, one of which named DELETING the artifact as the exit and the other unpublish. Each
was written when it was true, and each survived the change that falsified it because
nothing links them.
All five now say the same thing: a refused delete is recoverable in the sense that it can
be retried once the destination answers again, and in no other sense. There is currently
no action that accepts the exposure and clears the record, and giving an owner one needs
the positive absence probe tracked with the publication-existence contract.
A publication's handle pins a profile NAME, and four places claimed it pins the account.
One of them was make_external_id's own one-line docstring, which said it binds "the
resolved account" while the line under it interpolated the profile name. A profile name is
a local alias the owner can repoint at another account, so the claim was not a wording
slip: it described a guarantee that would have made a repointed profile harmless, and
instead a repoint means a withdrawal resolves the name to the new account, removes
nothing, reports success, and clears the record -- leaving the original copy public with
its handle erased.
All four now say what is actually pinned, and the resolver that every mutation path routes
through says both what it answers and what it does not. Binding the account id is the fix
and is deliberately not improvised here: the separator is partitioned on its FIRST
occurrence so a profile name may itself contain it, which makes appending a field
ambiguous, and six parse sites read the current shape. It is recorded with the
publication-existence contract at data-loss severity so the sequencing is a maintainer
decision rather than a sixth unilateral change to what a publication's identity means.
Refuse a single-artifact delete when a republish lands after the withdrawal.
The existing `created_at` guard compares artifact GENERATIONS, which catches a
delete-and-recreate under the same slug but not a re-publish: that is the same artifact, so
its `created_at` is unchanged and the guard passes. The new publication is a live public
copy whose record is the only handle able to take it down, and removing the artifact
dropped it.
The store already had the machinery -- `delete(refuse_if_published=True)` re-reads the
record inside the same lock that removes the artifact -- but the single-artifact path could
not use it, because on that path `delete_for_artifact` never cleared the record, so it was
still present on the ordinary successful path and the guard would have refused every
published delete. So the handler now clears the withdrawn record explicitly, which is what
turns "a record is still here" into a usable signal, and then deletes under the flag. Both
delete paths are now the same shape; the folder cascade already worked this way.
Mutation-verified in both directions: dropping the flag reddens only the new test, and
dropping the clear reddens three ordinary published-delete tests with 409 != 200, which is
the concrete reason the flag cannot simply default to True. The flag's docstring claimed it
existed for BULK callers because the single-artifact case was answered at the handler; that
rationale was falsified by this change and now states the real precondition.
Stop pointing users at unpublish as a way out of an unreachable destination.
Both refusal messages ended by offering "or unpublish first to accept that the published
copy may remain". That operation refuses on an unreachable destination for exactly the same
reason the delete just did -- it keeps the record unless a removal or a confirmed absence
releases it -- so the advice sent the owner to a second dead end and implied an escape
hatch that does not exist. `DeleteWithdrawal`'s docstring already said so correctly; the
strings the user actually reads did not.
Three sites, found by grepping the claim's phrasings rather than a symbol: the single-delete
UNREACHABLE message, the folder-cascade equivalent, and a comment in the cascade's keep
branch that described the owner "withdrawing or unpublishing" its way out. A fourth hit was
left alone deliberately -- the provider-switch comment requires an unpublish on a REACHABLE
destination, where it works.
Verify the sandbox headers policy is still ATTACHED when reusing an existing drive.
`_sandbox_headers_policy` already refuses to reuse a same-named policy whose CSP is not
exactly the sandbox one, which defends the moment a drive is CREATED. It cannot defend a
drive that already exists: the policy object can be perfect and simply not be the one
attached any more. Detach it, or point the default cache behaviour at a different policy,
and every artifact on this pooled domain is back to sharing one real browser origin -- the
single cost of pooling this module's own docstring says cannot be argued away. The reuse
branch re-asserted the BUCKET policy two lines earlier and never looked at this one.
A public publish now verifies the attachment and refuses on a CONFIRMED mismatch, naming
the policy to re-attach. An unreadable distribution config does NOT refuse: that third
state is the same one the enabled flag uses, because an install whose IAM policy was
narrowed would otherwise have a permissions gap reported to it as tampering. Withdrawal and
take-private are deliberately not gated on it -- refusing to REMOVE content because a
header is missing would strand the copy that header was containing.
Gated on `link_promised`, not `require_serving`: the sole production caller passes
`require_serving=False`, so gating there would have been dead code. Mutation-verified both
ways -- removing the check reddens only the two tamper tests, and failing closed on an
unreadable read reddens the new tolerance test AND the pre-existing
`test_unreadable_distribution_config_does_not_block_publishing`, which is what proves the
leniency was already a pinned contract rather than a new invention.
Also corrects `_distribution_enabled`'s docstring, which gave a false reason for correct
behaviour: it claimed the shipped IAM tier does not grant `cloudfront:GetDistributionConfig`.
The documented drive tier DOES grant it; the leniency is for installs whose policy was
narrowed or predates that tier.
Verify the sandbox attachment on EVERY transition into public serving, not just publish.
The previous commit put that check on the first-publish path only. A private -> public flip
reaches the drive through `_require_drive`, a DIFFERENT resolver, so it started serving bytes
on the pooled domain without the opaque-origin guarantee. Both review lanes found this
independently and they were right: gating the guarantee on how an artifact happened to
become public is not a guarantee.
The gate differs per method for a reason worth stating. In `_require_drive`, `require_serving`
IS the make-public signal -- its own comment already said so for the bucket policy ("the only
caller that asserts the network is make-public") -- so the check hangs off it there. In
`_ensure_drive` the sole caller passes `require_serving=False`, so the same choice would have
been dead code, and it hangs off `link_promised` instead.
Three paths stay deliberately ungated, each for the same underlying rule -- never refuse an
operation that reduces exposure. Withdrawal and public -> private are removals. `push_version`
replaces bytes in place and hands out no new link, so if the header were already missing that
artifact was already served without isolation before the push: refusing un-exposes nothing
and only strands its owner on the old bytes. Each of the three now has a test pinning that it
is NOT blocked, so a later tightening cannot quietly turn one into a refusal.
Keep the provider off the gateway boot path, and stop mislabelling a re-check failure.
Two findings the rebase surfaced, both genuinely in this change.
`register_publish_providers` runs inside platform bootstrap, before the socket is bound,
and it imported the provider module eagerly -- ~0.5s of cumulative import, because the
module reaches the deploy engine's profile registry, the artifact store and the validation
stack. That is work added to every install's time-to-ready for a destination most of them
never select, now that the drive is opt-in, and `no-new-work-on-gateway-boot-path` is a
written rule here. The registry was already factory-based and instantiates lazily, so only
the import needed moving: bootstrap now registers a factory that imports on first
selection. The key is spelled in `defaults.py` so bootstrap need not import the module to
learn it, and a test pins that the copy still equals PERSONAL_DRIVE_PROVIDER.
The notice re-check wrote its failures to `setSaveError`, the editor's save channel, whose
surrounding copy tells the user their edit did not persist. It did persist -- nothing was
being saved -- so the message sent them to re-save untouched content. The re-check now has
its own state, rendered beside the button that triggered it.
The boot test took three attempts and each failure is worth recording. It first passed WITH
the eager import restored, because `kiro_crew.publish` stays imported with `personal_drive`
bound as an attribute, so `from ... import personal_drive` resolves the attribute and never
enters the import system; the parent's attribute has to be deleted too. It then broke
`test_public_edition_registry_wires_the_drive`, because re-importing produced a second
`PersonalDriveProvider` class and the older reference failed `isinstance` against it; the
teardown now restores the SAME module object. Mutation-verified in both directions.
Declare the factory's return type instead of silencing the linter that caught it.
The previous commit annotated the lazy factory as `-> "PublishProvider"` and suppressed
the resulting flake8 F821 with a noqa, because the name is deliberately not imported at
runtime. That silenced the warning without making the annotation true: the name really was
undefined in this module, and mypy said so -- `Backend Lint & Type Check` failed on exactly
one error, `defaults.py:112: Name "PublishProvider" is not defined`.
The name now comes from the existing `TYPE_CHECKING` block, which costs nothing at runtime
and makes the annotation resolve for real, so the noqa is gone with it. A suppression that
hides a true report is worse than no annotation.
Pin the AWS ACCOUNT into a publication's handle, not just the profile name.
A profile name is a local alias. Repointing it at another account and then deleting an
artifact resolved the name to the NEW account, deleted an absent key there, reported
success, and cleared the record -- leaving the original copy public with the only handle
able to take it down erased. No concurrency and no failure were needed, just a config edit
and an ordinary delete, which is why this is fixed here rather than deferred with the
timing-dependent windows.
The handle becomes `<key>~<account>~<profile>`. The account goes in the MIDDLE, and the
middle field is read as an account only when it is twelve digits, because a profile name may
itself contain the separator. Appending instead would have been ambiguous, and counting
separators would misread a pre-binding `key~we~ird` handle as account `we` -- then compare
`we` against a live account id, mismatch forever, and refuse every operation on it. There is
one parser, and `split_external_id` keeps its two-field shape so its eight callers are
unchanged.
`_profile_for` is the one function every mutation path resolves credentials through, so the
verification lives there rather than at each caller -- the same reasoning that put the
sandbox-header check on the shared resolver. A mismatch refuses. So does an account that
cannot be read: `sts:GetCallerIdentity` is gated by no IAM policy, so a failure means
credentials did not resolve rather than a tier being narrow, and acting on an account we
cannot identify risks removing nothing while discarding the record. The account is read
before the handle is minted, so a publish that cannot confirm it fails before uploading.
A handle with no account predates the binding and keeps the older behaviour instead of being
refused; refusing would strand every publication made before this change for no gain.
Mutation-verified both ways, and the second one caught a bad test: disabling the mismatch
check reddens the repointed-profile test, while removing the twelve-digit guard reddened
NOTHING until the parser test covered `abc~we~ird` -- a two-field handle has no second
separator, so the guard never fires on the case I had asserted.
Route the re-check error through ErrorNotice, and record the new endpoints in the feature map.
The re-check failure rendered as a hand-written red span. `errors-use-error-notice` is a
written rule here, and unlike `PublishHub` -- which has seven such sites and no `ErrorNotice`
usage at all, on main as well as here -- `ArtifactDetailPage` already uses `ErrorNotice`
twice and had zero raw red spans before this change. Mine was the only one, so this is the
asymmetry the rule exists to catch rather than a component-wide gap. It carries the required
`{/* No hand-off: ... */}` comment: this panel sits beside an editable buffer that `askAgent`
would destroy by navigating away, and a failed re-check is retryable in place.
The feature map's Publishing row now lists the publish, unpublish, refresh, reprobe-notice
and sharing endpoints, and names both components that drive them. The row was owed once the
rebase brought `docs/feature-map/README.md` into the tree. Each endpoint was checked against
the router rather than transcribed -- a first draft cited a `publish/core` route that does
not exist.
|
/ai-review override gpt 362153a: neither is a false positive -- the isolation check fails open by design because GetDistributionConfig must be granted explicitly, so refusing would turn a narrowed policy into a permanent publish outage reported as tampering (pinned by test_unreadable_distribution_config_does_not_block_publishing), and the upload-before-persist window is a real open defect tracked on #8148, accepted as residual risk for a destination that is opt-in and not the default. Recorded deliberately rather than to clear a lane: this gate is not a required status check on this repository, so the override changes no merge condition. It is here so the verdict carries its reasoning instead of standing unanswered. The second finding is genuinely open. It needs a per-slug lifecycle lock spanning load, withdrawal and local delete, plus an expected-generation delete in the store -- the racing read happens in the delete handler above the publish seam, so wrapping the withdrawal alone would look like a guard while closing nothing. That is the contract change tracked on #8148, not something this PR quietly resolved. |
Human judgment recorded@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
3 participants
no linked issue: nothing here should close on merge. The two issues this body cites are cited for context, not as work this PR completes. #3599 is already closed and is the prior Artifact Deploy hardening it builds on. #8148 is the contract this PR deliberately defers work to, so a closing keyword on it would be actively wrong — merging this must leave it open.
1. What is the problem?
The artifact publish seam is fully built - a destination registry, provider-agnostic orchestration in
publish_sync, a picker in the dashboard - but the public edition registers no destination.DefaultPublishRegistry.register_publish_providers()returnedNone, soget_provider()raisedPublishUnavailableErrorand publishing was unavailable out of the box.The one publish path that did work reached a different mechanism (an app-declared provider) and used one bucket plus one CloudFront distribution per artifact, keyed on the artifact slug. That shape has three consequences:
Registering a destination also made a second problem reachable for the first time.
publish_synchad paths that only ever wrote a publication on success: a failed withdrawal cleared the local record anyway, and a visibility change never revisited the stored link. With no destination registered there was no publication to fail on, so none of it could be observed. This PR is what makes it observable, so it fixes it here rather than shipping a destination onto it.2. Why this issue matters to the user
Publishing is how an artifact leaves the dashboard. Today, on a stock build, the answer is "unavailable"; and where a publish did work, the user's first experience is a link that does not load, with nothing in the product explaining whether it is still propagating, was disabled by a security control, or simply failed.
The withdrawal defect is worse than unavailability, because it is silent and it points the wrong way: the user pressed un-publish, the product said it worked, and the copy stayed public with no local record left to withdraw it.
3. How our fix solves it
A new module,
src/kiro_crew/publish/personal_drive.py, implements the existingPublishProvidercontract, and the public edition'sDefaultPublishRegistryregisters it. Registering it is not sufficient on its own -- see "The panel reads both registries" below. Six changes outside that module are part of this PR: three behaviour changes inpublish_sync(withdrawal, the link a visibility change backfills, and an artifact delete now withdrawing its published copy), one optional field onPublishResultthat lets a destination report a link that is not usable yet without losing the publication, two additive parameters on the deploy engine, and the panel wiring. No new interface method, no new publish parameter, no core branching.The panel reads both registries.
PublishHubfetched onlyGET /api/publish-providers, which returns app-declared destinations plus the core deploy row and documents that built-in destinations "are registered frontend-side and are not returned here" -- but no frontend list of built-ins existed. So registering a destination in the core registry made it reachable from the API and MCP paths and left the Publish panel showing nothing, which is most of what "publishing is unavailable out of the box" meant in practice. The panel now also readsGET /api/artifacts/publish-providers?kind=<kind>and merges the rows. Three details are load-bearing: an id claimed by an app row keeps the app row, because an app may declare a row under a core destination's id and the pre-existing resolution for that clash is app-first; a core row publishes throughPOST /api/artifacts/{slug}/publish, never through the app helper, whose no-endpoint fallback is/api/deploy/deploy-- the per-artifact deploy machinery this destination exists to replace; and a missingavailableflag counts as available, since the field is documented as omitted by older gateways.A core row goes through the same consent gate, and offers no control it cannot honour.
POST /api/artifacts/{slug}/publishreturns norequires_confirm, so routing a core row through the panel's first step landed it directly in the success branch: content became world-readable on the first Publish click, andPublicPublishAckModal-- the blocking acknowledgment in front of every action that creates a publicly accessible website (#3599) -- never opened. A core row now enters the confirm step locally and publishes only from the acknowledged path; consent is about what is about to happen, not about a content digest, so needing no backend preview is not a reason to skip it. The TTL select is also hidden for a destination whosesharing_modeldeclares no expiration support: the core publish route carries no TTL, so offering the control meant a user choosing "72 hours" received a persistent public link. For the same reason the acknowledgment's exposure sentence is now supplied by the caller: its default namesrecallanddestroy, which are the deploy surface's actions, so a destination without them was telling the user their way out is something that does not exist for it. A core row says what actually ends the exposure: the link stays public until the published copy is withdrawn, and deleting the artifact requests that withdrawal. That wording went through two corrections, both worth recording because each earlier version named a control the user does not have: "private or unpublish" described the deploy surface's actions, and "until you delete the artifact" is false whenever the destination cannot be reached, since the delete then only requests a withdrawal that may not happen.An unavailable destination is still listed, and explains itself. The core endpoint already keeps a destination visible while
availableis false as long as it isinstallable(), on the grounds that hiding it makes it undiscoverable until the user sets it up by hand. The descriptor now also carriesinstall_hint, and the panel renders it in place: only a provider knows which action makes it available, and sending the user to the deploy setup page would describe a different destination's flow.Pooled infrastructure. One private bucket and one distribution serve every published artifact; each artifact is a single object under a served key prefix. The first publish builds the drive, and every publish after it is an upload plus an invalidation. This removes the propagation wait, the per-artifact quota cost, and the empty-origin window that gets a distribution disabled. The cost of pooling is that every published artifact shares one origin, so they share browser storage; see section 5.
Prefix-scoped bucket policy, re-asserted on every publish. The bucket also holds private objects, so the shared engine's whole-bucket grant would serve them. The provider writes its own policy granting CloudFront read on the served prefix only, plus an explicit
Denyon everything else. It is re-installed each time an existing drive is resolved, because both resources are tagged and therefore discoverable the moment they exist: had the policy failed once, the reuse path would otherwise hand out links to a bucket CloudFront cannot read - 403 for the life of the account, reported as success.The distribution is created carrying its final tags.
engine.create_distributiongains one optionaltagsargument; omitting it leaves every existing caller byte-identical (verified: the deploy suite passes unchanged). The drive passes its own tag set, so the distribution is never stamped with the deploy surface's site tag. Correcting the tags after the fact instead would need two more calls, a window in which the pooled drive is listed as a destroyable deploy site, and acloudfront:UntagResourcepermission the shipped least-privilege policy does not grant.Every published artifact gets an opaque origin. Pooling means one domain serves them all, and browser storage is keyed by origin, so without this one published document could read what another wrote to
localStorage. These documents are mutually untrusted -- they are authored content, and the drive's owner is not the only author of what ends up inside them. The drive therefore attaches its own CloudFront response-headers policy carryingContent-Security-Policy: sandbox allow-scripts allow-popups: scripts still run, links still open, and storage is simply unreachable, so there is nothing to share. It must be a header -- thesandboxdirective is ignored in a<meta>CSP by specification. This matches the posture the dashboard's own viewer already applies (it renders artifact HTML in an iframe withoutallow-same-origin), so no artifact loses a capability it had; the published copy was the one surface where the sandbox was missing.engine.distribution_configgains the optional policy id for this, additively, and the drive finds-or-creates the policy so a second drive or a retried create reuses it.Only a first publish may create infrastructure. Push, sharing changes and withdrawal resolve an existing drive and report a missing one. Routing them through the create-capable path meant a discovery miss - a tag removed by hand, a transient answer from the tagging API - made a withdrawal build a fresh bucket and distribution, delete keys that were never in it, and report success while the original object stayed public.
Withdrawal is no longer best-effort at the seam.
publish_sync.unpublishpreviously cleared the local publication whether or not the destination delete happened. That block is the only handle to a served copy, so it now raises and keeps it, leaving the withdrawal retryable. This is a behaviour change for every destination and every edition, not just this one: an un-publish whose destination call fails now surfaces an error where it used to report success. Deleting the artifact remains the way to drop a record for a destination that is gone for good, and that path does attempt the destination withdrawal first -- see the delete paragraph below for when it proceeds and when it does not.A visibility change backfills a link the private publish could not derive. A destination whose link is derived rather than returned can only produce a usable one once the artifact is served, so a private publish stores none. Nothing else revisited the field, so turning an artifact public left it public with no link at all. The fill is additive - never replaces, never clears - so a destination that returned its own URL keeps exactly that URL.
Mutations on one artifact are serialized. Every mutating path is read-then-write against S3, and the seam does not serialize per artifact. A push landing between a move's copy and its delete had its bytes deleted with the source, leaving the record naming a version the drive no longer held and every later push in permanent conflict.
Error messaging for the states that produce a dead link. Both are invisible from the URL, so a publish that returned a link without mentioning them would look like a product that hands out broken links:
Deployed-> says it is still rolling out, that nothing was lost, and that the same link will work shortly;Enabled: false-> says the delivery network is disabled, that this is not a state the product sets, and what to do about it;An unreadable enabled-flag is treated as no objection, so a permissions gap does not masquerade as a disabled distribution.
Neither dead-link state may cost the withdrawal handle. Both are discovered only after the object is uploaded, and the orchestration records the publication only when
publishreturns - so raising there left a world-readable object with nothing to withdraw it by, while the message it raised claimed nothing was lost. A fresh distribution isInProgressfor minutes after creation, which made that the normal outcome of a first public publish.PublishResultgains an optionalnotice, recorded as the publication'slast_error, so the state is reported where destination trouble already surfaces and the handle survives. The field defaults to empty, so no other destination changes behaviour.Discovery asks each API that documents support for the resource it is looking for.
resourcegroupstaggingapireturns only resources "located in the specified AWS Region", and CloudFront's own documentation states Tag Editor and Resource Groups are not supported for CloudFront, listing only the CloudFront API's own tag operations. A single regional call therefore finds the bucket and silently misses the distribution -- and that gap is not harmless: discovery reports a half-built drive, which sends a HEALTHY drive down the partial-create recovery path, builds a SECOND distribution, and re-points the bucket policy at it, so every link already handed out answers 403; the second tagged distribution then makes discovery ambiguous permanently. The default region is not us-east-1, so this was the normal path rather than an edge case. The bucket is now resolved regionally, and the distribution is resolved by a tag query in us-east-1 with a fallback to CloudFront's own tag API. The tag stays the trust anchor in both paths - no origin-matching heuristic is introduced - and the recovery path still fires for the state it was written for, a bucket tagged by a run that died before its distribution existed.Withdrawal is bounded, not merely submitted. Taking an artifact private or unpublishing it deletes the object and submits a CloudFront invalidation, and the invalidation is deliberately not awaited: one takes minutes, so awaiting it would block a user-facing withdrawal, and a timeout would force the caller to report failure after the object is already gone - the lossy shape this module is careful to avoid. That leaves cached copies as the exposure, so every upload now carries
Cache-Control: max-age=300. Without it objects inherit the distribution's cache policy default (Managed-CachingOptimized, 24 hours), so an edge that had served a public artifact could keep serving it for a day after the user withdrew it and was told it was gone. The invalidation stays the fast path; the header is the guarantee.Deleting an artifact withdraws its published copy. The delete handler calls
publish_sync.delete_for_artifactBEFORE the local delete, using the publication it already read for its version capture. Before this, deleting a published artifact removed the local record and left a world-readable copy served with nothing left to withdraw it by. The ordering is chosen for its crash residue: die between the two steps in this order and the copy is withdrawn while the artifact remains, which the user simply deletes again; die between them in the reverse order and the record is gone while the content is still public.delete()removes the artifact directory by slug under the store lock rather than writing back a value read earlier, so a network round trip ahead of it widens no compare-and-swap window. Whether a destination failure blocks the local delete: it now always does, and this is a user-visible change to an existing route. An earlier revision of this PR split the two halves — a destination that could not be reached at all proceeded, on the grounds that it was the gone-for-good case and refusing would leave an artifact its owner can never delete. That split was withdrawn, because "cannot be reached" is not evidence of "is not being served": credentials expiring or a network partition looks identical to a bucket that no longer exists, and the copy may still be world-readable. SoDELETE /api/artifacts/{slug}gains a new502outcome, in which the delete is refused and the publication kept, for both halves — a destination that answers and rejects the removal (FAILED), and one that cannot be reached or is not registered in this edition (UNREACHABLE). The record is the only handle able to reach a copy that may still be served, so it is not dropped on anything short of a confirmed withdrawal or a confirmed absence. The cost is worth stating plainly: a published artifact whose destination refuses or cannot be reached cannot be deleted until that changes, and a profile holding the publish permissions but nots3:DeleteObjectis exactly that shape — no shipped IAM tier grants it yet (section 5). There is also no second way out: unpublishing keeps the record too unless a removal or a confirmed absence releases it.delete_for_artifactnever raises either way; it reports which outcome occurred and the handler decides, so provider resolution and the availability probe failing on an unregistered destination cost a log line rather than an exception.One registry key, opt-in rather than default, and no account picker. The edition registers exactly one destination, under the drive's OWN key rather than the seam's default key. That is what makes it opt-in:
publish_syncresolves a publish that names no destination through the default key, which stays unregistered, so an unnamed publish gets the same 503 as an edition with no provider at all, while the picker still lists the drive and a caller can name it. This is a deliberate reduction from the original shape of this change, and the reason is stated in "What is deliberately not in this PR" below. Per-account keys were tried and withdrawn: a provider id is validated against^[a-z0-9-]{1,32}$at the HTTP boundary, so an account-qualified key renders a picker row that rejects its own submission, andlist_providersdoes not dedupe, so a second key for one provider renders twice. A new publish therefore uses the profile registry's default; an existing publication pins the AWS account id that holds it into itsexternal_id, and every mutation path verifies that pin before acting, so neither a moving default nor a repointed profile name can send a withdrawal to the wrong account.Shared folders are deliberately absent. The provider declares the
SHARINGcapability, which means it can change an artifact's visibility - not that it has a per-principal grant list.sharing_modeldeclaressupports_shared=False, so the UI never offers a grant list it cannot honour, and a request carrying recipients is refused explicitly rather than silently downgraded to world-readable. Adding shared folders later is a capability declaration plus its implementation, not a redesign.4. What tests we did
test/test_publish_personal_drive.py- 96 tests, no AWS reached (the engine call surface is stubbed, so these assert the provider's own decisions).test/test_publish_sync.pygains coverage for the two seam changes.display_nameis vendor-neutral.available()provably makes no network call; a named-but-uncached account never borrows another account's domain.test_a_reachable_withdrawal_failure_keeps_the_artifact_and_its_handleasserts a reachable rejection answers502and KEEPS the publication, andtest_an_unreachable_destination_refuses_the_deleteasserts the same for a destination that cannot be reached. That second test previously asserted the OPPOSITE and its docstring records the reasoning that was withdrawn. Withdrawal never raises through either mouth a wired caller can hit.last_errorwhile the publication is still stored.Cache-Control, so a withdrawn artifact cannot be served from an edge for the cache policy's 24-hour default.502), and a destination that refuses the removal (reported FAILED, delete REFUSED with502).test_an_unregistered_destination_refuses_the_delete_and_the_handler_adds_no_guardalso pins that the handler adds no guard of its own: a stub that raises propagates straight through it, so the decision genuinely lives indelete_for_artifact.website/src/test/): the two registries merge with app rows winning an id clash; an unavailable row is listed and renders its remedy without being selected; a core row does NOT issue a publish request on the first click; no TTL control is offered for a destination that cannot expire a link.Gates run on the changed files only:
black --check,flake8,mypy, and the repo's own black-baseline gate all clean. The deploy suite (707 tests) passes unchanged, which is what establishes thecreate_distributionparameter as additive.Not verified: no live AWS publish was performed from this branch, so the first real drive creation is still untested end to end.
Visual evidence. Recorded in an isolated pod with
AWS_CONFIG_FILEpointed at an empty file, so the account list is empty and no real account data is on screen:temp-screenshots/pr7199-evidence.mp4(18s, silent). The row label and the remedy text were read out of the DOM rather than eyeballed, and a request interceptor confirmed no publish request is issued before the acknowledgment.The clip predates the consent-copy fix in this round: its acknowledgment frame still shows the default persistent sentence naming
recall/destroy, which is what the fix replaces for this destination. What the clip demonstrates - the destination appears, explains itself unconfigured, and cannot publish without the acknowledgment first - is unchanged.5. Any other suggestions on the work
What is deliberately not in this PR
The publication-existence contract, and with it the default. Five review rounds on this branch converged on one question rather than five: what proves a publication exists, or no longer exists, and which paths may act on that. Answering it needs agreement across the publish engine, the artifact store and the folder store — three lock domains that do not nest, and whose cross-lock coordination this change set already declined once because of ordering deadlock. It also needs a positive absence probe, because resolving the destination by tag can only report that a lookup failed, never that a resource is gone.
Two windows in this PR depend on that contract and are therefore left open rather than patched again: a first publish can race a concurrent delete around the engine's per-artifact publish lock, which the delete paths do not take; and an unpublish against a drive that was deleted under a still-registered profile refuses instead of releasing the record. Both are recorded on this PR with the round table, and both are the same issue: #8148.
Holding the drive to opt-in is what makes that acceptable to ship: every one of those windows is now reachable only by someone who deliberately chose this destination, instead of by every user on the default path. The feature ships; the exposure does not. Flipping the key to the seam's default is the last step of the contract work, not a step of this change.
An earlier round declined the "keep it non-default" suggestion on the grounds that it removed this PR's purpose. That was right on the information available then. What changed is knowing that shipping it as the default requires the cross-store contract — so it is no longer the same trade.
A publication's handle pins the AWS ACCOUNT that holds it, not just the profile name. An earlier revision of this PR pinned only the name, and that was a real exposure rather than a nicety: a profile name is a local alias, so repointing it at another account and then deleting the artifact resolved the name to the new account, removed nothing, reported success, and cleared the record — leaving the original copy public with the only handle able to take it down erased. It needed no concurrency and no failure, just a config edit and an ordinary delete, which is why it is fixed here rather than deferred with the timing-dependent windows. The handle is now
<key>~<account>~<profile>; the account sits in the middle and is recognised by shape (twelve digits), because a profile name may itself contain the separator, so appending would be ambiguous and counting separators would misread a pre-bindingkey~we~irdhandle as accountweand then refuse every operation on it forever._profile_for— the one function every mutation path resolves credentials through — compares the pin againststs:GetCallerIdentityand refuses on a mismatch, and also refuses when the account cannot be read at all, since that call is gated by no IAM policy and acting on an unidentifiable account risks removing nothing while discarding the record. The account is read before the handle is minted, so a publish that cannot confirm it fails before uploading.Other follow-ups
recallordestroythere from wiping every published artifact at once - but it also means no product action deletes it. After the last un-publish the bucket and distribution remain in the owner's account, costing money, removable only from the AWS console. A publish-surface teardown ("remove this drive, refusing while anything is still published") is the missing piece and should land near this, not open-ended.kirocrew-web-*namespace, but itspublic//private/object prefixes andcloudfront:ListResponseHeadersPolicies/CreateResponseHeadersPolicy/ListDistributions/ListTagsForResourceare outside the generated least-privilege policy indeploy/iam.py. A publish tier modelled on the existing_drive_statements()is the fix, and it should land with or immediately behind this change.engine._checked,_harden_bucket,_oac_nameand_trimmed_stderr; a deploy-engine refactor could break publishing silently. Promoting them is a public-surface decision about that module and belongs with the publish IAM tier, which touches the same file.