Repository navigation
Hand the App Store Connect key to the clients through the environment - #297
Merged
Merged
Conversation
… to App Store Connect There is no iOS simulator runtime on Jay's Mac, so the App Store screenshots for the CodeCaps iOS companion have to come from a GitHub runner. The existing swift-ci companion job already builds the app and seeds a quota snapshot, but it captures at whatever size the runner's first iPhone happens to be and never asserts the result, so its images are not usable as App Store assets. This adds a dedicated capture script and workflow. The script builds the companion for one iPhone and one iPad simulator, seeds each with a realistic multi-provider quota snapshot, captures the quota list, the settings sheet and the platform-order editor, and asserts every PNG is exactly the pixel size App Store Connect accepts before the file is handed on. A screenshot with an alpha channel is flattened to JPEG, because Apple rejects transparency. The upload to App Store Connect is a separate, explicit step rather than part of the workflow, so a re-run can never quietly add duplicate screenshots to a live version. The new asc-api.mjs subcommand creates or reuses the screenshot set for a display size, reserves each screenshot, sends the bytes to the pre-signed URL Apple returns, and then commits the reservation and reports the asset delivery state. The display type is the App Store Connect enum: APP_IPHONE_67 carries the 6.9-inch iPhone size and APP_IPAD_PRO_129 the 13-inch iPad size, both verified against the live API. Refs CC-169
…s one card The seeded windows are the screens App Store reviewers will see, so each provider has to land on a single clean card. Antigravity is dropped: the app pools its windows into per-model sections rather than one platform card, so it would have read as "Antigravity · Third-Party" in the listing screenshot. Cursor's two allowances become one 1-month window, because a second window of the same cadence is folded in as a duplicate source instead of getting its own row, and the window tokens are now only the ones QuotaWindowVocabulary recognises, so every card shows a clean cadence caption. Refs CC-169
Four defects, all of which would have wasted a capture run: - The first screenshot was taken before the app was ever launched, so it recorded the simulator home screen. The app is now launched before every capture, with the launch argument that opens the screen being shot. - assert_screenshot logged to stdout, and its stdout is what the caller captures to learn the accepted file path, so the log line became part of the path and the following mv failed. log() now writes to stderr. - The expected pixel size was hardcoded to 1320x2868 and 2064x2752 while the picker could fall back to a device that renders at another size, so the assertion would fail on a runner without the first choice. The size is now measured from the capture, every file has to agree on it, and it has to be a size App Store Connect accepts for its display class. The output files are named for the class they actually are. - The upload step could be given the wrong display type by hand, and there is no APP_IPHONE_69 or APP_IPAD_PRO_13 in the enum to be caught by. The upload now takes "auto" and reads the pixel size out of the PNG to pick the set: 1320x2868 and 1290x2796 go to APP_IPHONE_67, 2064x2752 to APP_IPAD_PRO_129, 2048x2732 to APP_IPAD_PRO_3GEN_129. Also stops the workflow cancelling a main-branch run for a commit that nothing replaced, which reported cancelled checks to anything polling the workflow API. Refs CC-169
The capture script took a device family argument and never used it, so a simulator that rendered at the other family's size would have been captured, named and uploaded into the wrong screenshot set without a word. The display class is already derived from the measured pixels, so the check is a comparison of two things that are both already known. It fails the run instead of quietly producing a mismatched set.
Xcode names some iPads with their storage variant, so "iPad Pro 13-inch (M5)" never matches a device called "iPad Pro 13-inch (M5) (16GB)". On a runner that only has the current runtime there is no other iPad to fall back to, so the capture would have found no iPad at all and failed after an hour of runner time, having already spent the time. The picker moves into its own module so it can be tested, and it matches on a prefix so the storage variant does not hide the device. The tests cover the Xcode 27 names, the preference order, and a device that is not available.
The checked-in CodeCapsCompanion.xcodeproj is stale. It is missing three shared QuotaCore files the companion compiles against, so a build from it fails to compile. The capture only regenerated when project.yml was newer than the project file, and a checkout gives both the same mtime, so the comparison came out false and the script built the stale project. xcodegen takes a second, so the project is now always regenerated from project.yml.
The upload read the pixel size of the first file and sent every other file to that file's screenshot set, so an iPhone and iPad capture in one call would have filed the iPad images into the iPhone set. Every file's size is read now and the call stops if they do not all belong to the same set, naming the set they do belong to. The check runs before the first reservation, so a wrong call costs nothing against the live version.
The capture was writing a blank window. The companion takes about eighteen seconds to show its first frame on a cold simulator and about nine on a warm one, so the fixed four-second sleep was firing long before anything was on screen. The app was never broken: a ten-second sleep showed the full quota list. The script now polls. It takes a frame, measures how much of it differs from the background, and keeps the first frame that clears the bar. A window with nothing but its status bar measures about 0.3 percent, a half-drawn screen about 4, and a finished screen between 40 and 55, so the bar sits at ten percent, clear of both ends. The measurement lives in screenshot_ink.py behind nine tests. Three more defects went with it. The old check deleted the file it was checking inside a command substitution, so the delete printed the file name into the captured value and the run aborted. The output was renamed with a move onto itself, which did nothing. And the file was written as a PNG with an alpha channel, which App Store Connect rejects; the simulator writes JPEG, which cannot carry alpha, and it is converted back to an alpha-free PNG. The captured files are now named after the display set they belong to, so an iPhone file and an iPad file cannot be uploaded into the wrong set. The snapshot is re-seeded before every launch, so the time on a card is the time of its own shot. Apple's screenshot specification has no 2000 x 2664 device, so that size is gone from the script and from the upload step; the 12.9-inch Pro is 2048 x 2732. A compiled Python cache file was tracked by mistake and is now ignored.
…ab on request The source-scope picker showed its stored raw value, which is always "This iPhone". On an iPad the reader therefore saw a control that said it was looking at an iPhone, which is the one thing the control is there to tell them. The stored value stays "This iPhone" so a saved preference is not lost, and a separate display name follows the interface idiom: iPhone, iPad, or Mac on the Mac build. The screenshot capture also needs to reach a tab of the settings sheet that has no launch argument of its own, so "-openAlarms" opens the Alarms tab the same way the existing arguments open the sheet and the platform-order editor. Both changes carry a test.
The first capture in the loop ("fleet-status") leaves args empty, and the
runner runs the script with macOS system Bash 3.2. Under `set -u` that
version treats "${args[@]}" on an empty array as an unbound variable, so
the loop exited on its first iteration and no PNG was ever written.
Use the ${ARR[@]:+"${ARR[@]}"} idiom the rest of scripts/ios-fleet already
uses (ship-testflight.sh:1256), which expands to nothing when empty and to
the quoted arguments otherwise.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The capture runs on a macOS runner whose system Bash is 3.2, and Bash 3.2 reads an empty array under "set -u" as an unbound variable. The first capture passes no launch arguments, so the unguarded form aborts the run on the very first screen, after the build has already spent its time. The autofix bot found it and changed the expansion to the guarded form. This adds a contract test so the same class of bug cannot come back. It checks that the script still parses, that every expansion of the launch-argument array is guarded, that the guarded form survives "set -u" with an empty array, that no Bash 4 builtin has crept in, and that the capture still waits for a drawn frame instead of sleeping.
Every finding raised on the App Store screenshot tooling is addressed here. Timeouts: every App Store Connect call now carries a 120s AbortSignal. Node's fetch has no default timeout, so a call that never answered would hang the upload with its reservation still open. Orphan reservations: a failed upload now DELETEs the reservation it opened, on every failure path. A screenshot set holds ten entries, so an open reservation is not cosmetic: it pushes the real screenshots out of the set. The delete is awaited and its outcome is reported, so the caller learns whether the entry actually went away. Error bodies: an App Store Connect failure no longer echoes the raw response. Only the JSON API error titles and details are used, with runs of 24 or more token-like characters redacted. A rejected key is named by variable -- ASC_KEY_ID, ASC_ISSUER_ID, ASC_KEY_P8 -- and never previewed. Shape guards: hand-written isPlainObject / requireObject / requireArray, because the scripts in this directory are dependency-free. Ink threshold: the quota list and the settings sheets are measured against separate minimums (10% and 6%), with a stability fallback that keeps a sparse frame once two polls agree. The background value is found from a stride-16 sample confirmed against the whole channel, which is 39x faster than counting every byte, with the exact mode as the fallback. Build once: both simulators install the same bundle, so the companion is compiled once instead of once per device. Workflow paths: the trigger lists the eight QuotaCore files the companion actually compiles, plus the two capture helpers. A broad Sources/QuotaCore filter would capture UI the shipped build does not contain, which is worse than a stale capture. New test: scripts/ios-fleet/test-asc-upload.mjs drives the real script against a local server and asserts that the reservation is dropped on every failure path and that no response body is echoed. It fails if the delete is fired without being awaited.
…k count The single-pass build writes to "$DERIVED_ROOT/app", so the per-device capture directory "$DERIVED_ROOT/$label" is no longer created as a side effect of building the app. capture_on() still writes its raw frame to "$derived/$label-$name.jpg", so `simctl io screenshot` had nowhere to write, wait_for_render saw an empty file on every poll, and the run died at the 180s timeout. Create it explicitly. Also correct the contract-test summary: it claimed 8 checks passed and there are 7. Co-Authored-By: Claude Code <noreply@anthropic.com>
The width and height sit at offsets 16 and 20 of the PNG header, but the only precheck on an upload argument was existsSync, so a file shorter than 24 bytes reached readUInt32BE and raised an uncaught RangeError. That surfaced as a stack trace and left the run exiting on the unhandled rejection rather than on the named exit 2 that every other bad input in this loop already gets. Reading the bytes once and rejecting anything too small to carry a signature and an IHDR header puts the failure back on the existing path. The bytes are reused by the upload loop below, which read the file a second time for the body. Co-Authored-By: Claude Code <noreply@anthropic.com>
Answer the unresolved review findings that are genuine defects: - measure_ink swallowed screenshot_ink.py's exit status. The helper exits 1 below its own default minimum, which is exactly the blank-frame case the poll loop exists to survive, and under `set -e` the failing command substitution aborted the whole capture with a bare exit 1 and no ::error:: annotation. - ASC_API_BASE could redirect every request carrying the ASC-signed JWT to an arbitrary host. Only a loopback override is honoured now, which still covers the tests. - api() let a TimeoutError escape instead of returning a non-ok response, so the commit path skipped discardReservation() and left a reservation open in a ten-entry set. - The post-POST guards exited without dropping the reservation they had just created; they now discard it when an id is available and name the set when one is not. - describeError() read .title/.detail off unvalidated elements of the service's errors array, which throws on a null element and replaces the safe message with a stack trace. - Removed the shadowed module-level REQUEST_TIMEOUT_MS. The remaining threads were already addressed by 597996e or the following autofix rounds, and were left alone: the launch-before-capture, log-to-stderr and measured-pixel-size findings are in the current script, the workflow concurrency finding is out of bounds here, and the stable-frame floor and the mixed-upload refusal are deliberate documented behaviour. Co-Authored-By: Claude Code <noreply@anthropic.com>
Nine findings from the last review pass, all addressed here. Transport failures are no longer server verdicts. A request that never reached App Store Connect comes back from api() marked networkError, and it is reported as one: latest-build-seq and ensure-tf-ready say the request never arrived instead of saying the app was not found, and the newest-build probe keeps "unknown" rather than claiming the app has no builds at all. The host override accepts the production host explicitly, so a job that pins the host it would otherwise use is not a redirect anywhere new, and a trailing slash is stripped so the prefix cannot double the separator. The check moved out of module scope into main(), which has a catch handler, so a bad value is a named failure instead of an uncaught stack trace. Failures no longer lose their own message. Every failure path sets process.exitCode and returns, so Node drains stdout and stderr before the process ends; a pending write to a pipe is not flushed by process.exit(). The two guards that cannot return from main() throw a named failure that the handler prints without a stack trace. The capture gate no longer has a way around its own thresholds. The stable-frame path was a second way to accept a frame, gated on a global floor below the per-screen minimums, so a navigation bar and nothing else could be accepted for the quota list. It is now a second condition on the same thresholds rather than an alternative to them, and RENDER_STABLE_FLOOR is gone. To make that check possible, screenshot_ink.py reports two numbers: the share of the whole frame that is not background, and the share of the region below the header, which is where a screen's content lives. A frame that has drawn only its navigation bar has ink in the header and none below it, so the content share is zero and the gate rejects it however much of the frame it covers. That is the case a whole-frame threshold could not see. A failed measurement is no longer swallowed. measure_ink keeps the exit status: 1 is the blank-frame case the poll loop has to survive, and 2 is a measurement that failed, which stops the run with the real error instead of spending the whole render budget on every screen. New test: scripts/ios-fleet/test-asc-client.mjs drives the real client against a local server and a closed port. It asserts the production host is accepted, a host that is neither is refused by name, a trailing slash does not double the path separator, a transport failure is not reported as app not found, and a payload larger than a pipe buffer is not truncated. Every one of those was checked by reverting the fix and watching the test fail. test-capture-script.sh now sources the capture and calls its gate with real numbers: a navigation bar with no content is rejected for every screen, a half-drawn screen is rejected, and the finished quota list and settings sheet both pass.
Five findings from the last review pass, all addressed here. A timeout and a transport failure are different causes. api() now says which one happened, and describeError says so too: a call that never answered is reported as a timeout, and only a call that never got there is reported as never having reached App Store Connect. The timeout was computed and then thrown away, so the operator was sent to check the network when the real cause was a slow App Store Connect. A rejected key is no longer reported as a missing app. Every failure the service reports now goes through describeError, and "app not found" is only said when the response was fine and carried no app. The newest-build probe checks the shape of the response before reading it. A malformed answer leaves the reading unknown instead of becoming a build version. A BMP with a short header is rejected by name. struct.error is not a ValueError, so it escaped the handler and landed on exit 1, which the capture reads as a blank frame, and the capture then spent its whole render budget on a frame it could never measure. The reader now says the file is too short, and struct.error is caught as well. The contract test no longer passes its own values positionally. The capture script reads its output directory from "$1", so the gate values were landing there and leaving stray directories behind. They travel in the environment now, and the capture takes OUT_DIR from the environment when there is no argument. A check asserts that sourcing the capture leaves nothing outside the directory it was given. New in test-asc-client.mjs: a call that never answers is reported as a timeout, and a rejected key is reported as a rejected key. The request timeout is overridable so a test can drive a real one.
The shape guard added in debf87a folded a malformed-but-successful probe into "none", because a body that fails the guards produced an empty rows array and `version || "none"` then asserted that the app has no builds at all. That is the same misreport the original finding was about, one level up. The three readings now stay distinct: a version is the actual newest build, "none" is the service reporting an empty array, and "unknown" is an answer that could not be read or did not have the expected shape. Co-Authored-By: Claude Code <noreply@anthropic.com>
The review asked for the masked first-eight/last-four preview of the key when App Store Connect rejects a credential, and I had pushed back on it. The push-back was wrong. The preview is the owner's rule of 2026-07-16, and the other fleet apps already use the same shape, maskApiKeyPreview in Socratic.Trade and buildKeyPreview in Usage Monitor. The reason it exists is that there is otherwise no way to tell which of several keys is the one in play, and pasting a value to find out is never the answer. So the rejection message now carries the fleet's masked preview of the key in play, and the key id beside it. The key id is a public identifier: it travels in the header of every request to App Store Connect, and it is the part that actually says which credential was refused. Nothing else about the handling of a credential changes. The response body is still never echoed, and the preview is computed from the key on disk rather than read out of anything the service said. test-asc-client.mjs asserts the preview and the key id are in the message.
Three findings from the review round, all in scripts/ios-fleet/asc-api.mjs. The app-lookup failure branch had replaced `app not found for bundle (HTTP N)` with describeError(apps), which returns the JSON API detail and never the status for anything other than 401/403. ship-testflight.sh:441-445 documents these `latest-build-seq:` lines as carrying Apple's HTTP status, and forwards them to the log; without it a 429, a 500 and a 404 all read the same. The status is now appended, omitted only for a transport failure, which has none. The same branch's shape guard checked that the app entry was an object but not that it carried a usable id, so `apps.parsed.data[0].id` could be undefined and every query below would send filter[app]=undefined and fail with HTTP 400 -- reported as a query the client built wrong, the same misdiagnosis the guard was added to prevent. The id is now required to be a non-empty string, in both the latest-build-seq and the ensure-tf-ready branch, which had the identical pattern. ASC_REQUEST_TIMEOUT_MS is now clamped to 2^31-1. AbortSignal.timeout does not accept a larger delay: it throws a RangeError inside api(), where the generic catch reports it as a transport failure, and at the raw fetch on the ensure-tf-ready path it escapes main() as a raw stack trace with exit 1. test-asc-client.mjs (8), test-asc-upload.mjs (5) and test-asc-credentials.mjs (7) pass. Co-Authored-By: Claude Code <noreply@anthropic.com>
Submitting is the one call in the client that cannot be undone, so the things that have to be true first are checked here instead of being discovered from Apple's refusal: the version carries its screenshots, a build is attached to it, and its versionString matches that build's marketing version. The review submission's items cannot be set from here. The relationship allows GET_RELATED and GET_RELATIONSHIP only, and a review submission is created with the app relationship, so App Store Connect binds the version itself once the version is submittable. That is also why the command lists the app's review submissions and tries each one: reviewSubmissions does not allow DELETE, so more than one can exist and the wrong one refuses. New test: scripts/ios-fleet/test-asc-submit.mjs drives the command against a local server and asserts that a version with no screenshots is not submitted at all, and that a ready one is submitted with submitted=true and platform IOS.
The 401/403 diagnostic previews the PEM envelope, which is the same for every .p8, so the preview named none of the keys the operator may hold. It also sliced the trailing newline into the preview, which split the diagnostic into two lines; ship-testflight.sh forwards only the lines that start with "latest-build-seq:", so the half carrying the HTTP status was dropped from the fleet log. The preview now comes from the key material, trimmed, so it discriminates between keys and stays on one line. The test now compares two keys' previews instead of recomputing the same formula from the same file, which could never catch this, and asserts the forwarded line still carries the status. Co-Authored-By: Claude Code <noreply@anthropic.com>
App Store Connect pairs a build with a version by their marketing version, so a version whose versionString is still the one from the previous build is refused. That is a cheap mistake to make and an expensive one to find out about from Apple, so submit-review checks it and says which value is right. The build's marketing version comes from its preReleaseVersion, which is not in the build's own attributes, so the build is read with that included. A third test covers it: a version that says one thing and a build that says another is not submitted, and the refusal names both.
Main moved two commits ahead while this branch was in review, with the alert-delivery work (#276) rewriting the two companion files this branch also touches. Both sides are kept. CompanionQuotaModel.swift: the iOS-only UIKit import and the CompanionInAppAlert struct that came with the alert work. CompanionContentView.swift: the settings tab is opened by either launch argument, -openAlarms (the screenshot capture's) or -openAlerts (the one the alert work introduced for the same tab). The test that pins the behaviour now checks both spellings and the tab they select. The source-scope label and the alarms launch argument are unchanged, and the whole suite is green: 480 tests, 0 failures.
Seven findings on the command, all addressed here. The screenshot check is per device family, not a total. App Store Connect wants a screenshot for every family the app runs on, and a pooled count lets three iPhone images and an empty iPad set through as six. The counts are kept per display type and reported per type, and a family with no image is named. A screenshot set that could not be read stops the submission. A failed read is not an absent set, and treating it as absent would let an irreversible submission go out on a read that never happened. A review submission is only submitted if it is bound to this version. A submission can be bound to another one, and submitting that would send the wrong build to review. The platform is an argument, so the same command works for the Mac app, and a platform that is not IOS or MAC_OS is refused by name. The reads that feed the checks are checked for success before their contents are used, so a failed read is reported as a failed read. Only the report goes to stdout. A wrapper that parses stdout as JSON cannot be broken by a progress line, so the progress moved to stderr. The report's own fields are shape-checked before they are printed, so a response that does not look like a review submission is a failure rather than a report of nothing. The test now covers all of it: an empty iPad set beside three iPhone images, a set that cannot be read, a submission bound to another version, a version string that does not match the build, the platform argument, and that stdout carries the report and nothing else.
The secret scanner reads a key-shaped block when one BEGIN marker and a later END marker sit close enough together in the same file. These fixtures are synthetic and are never a credential, but a squash merge lands the whole file in one commit, so the scanner sees both markers at once and reports the file. The allow marker says so where each fixture is.
The widget builds every window label through
QuotaWindowVocabulary.windowLabel(...), which rewrites the label into the
hyphenated cadence copy ("7-day window"). A qualifier matched from prose
words in the label therefore never matched a label the widget renders, so
two bars that shared a period both printed an empty qualifier and the
owner-approved "weekly" / "additional" pair never appeared. Three Kody
findings on the widget bar qualifier are fixed together.
The qualifier is now named from the canonical cadence the caption row
prints, and the label is consulted for the extra allowance alone. The
cadence group resolves the window's own cadence first and falls back to
the label only when the cadence field cannot be resolved, because the
group decides whether two bars share a period while the caption token
beside the bar prints the cadence field. A group that disagreed with the
token gave a bar a qualifier naming a period its own token contradicted.
An extra allowance is looked up without its label, so it keeps the period
it doubles instead of collapsing to "Other limit", and the canonical
labels now round-trip through the cadence lookup, so "1-month" resolves
like "5-hour" already did.
A caller that names a qualifier on a window keeps it. The sibling
resolution only fills in a qualifier the caller left blank, so the public
parameter no longer disappears on a platform with two or more windows.
The tests drive the production label shapes ("7-day window", "5-hour
window", "1-month window", "Additional allowance") rather than hand-written
prose, so the pair is pinned on the labels the widget actually renders.
… shell Closes the three review findings on the credential path: the reader wrote private key material to stdout, the ship script let a file credential bypass environment-first resolution, and the two CI callers provided only a path. The hosted iOS ship workflow stages the App Store Connect key to a mode-600 file and publishes only its path. The shell layer read that file into a shell variable before handing the key to the client in the child's environment, so the key was in the shell's memory, in a command substitution, and in the shell's own environment for the length of the run. The reader now execs the consumer, so the key goes from the file straight into the consumer's environment and never enters a shell at all. asc-key.mjs gains an exec command. It opens the path once with O_NOFOLLOW, reads from that same descriptor, and spawns the consumer with the key in the consumer's environment. It reports a path that names nothing apart from every other failure, and it passes the consumer's exit code on, so a missing key and a failing consumer stay distinguishable. The read and validate commands no longer print the key. A key on stdout is a key in every log that captures it, so they report whether the file holds a usable key and nothing else. The order of the credential sources is now the environment first and a named file second, in every consumer. A disk credential never overrides the environment, because a stale or foreign value in an operator's shell would sign the upload with the wrong key and App Store Connect would answer 401 without saying which input was wrong. The two hosted-runner consumers use the new command, the standing-testers step in ios-ship.yml and run_client in ios-install-appstore-profiles.sh. ios-enable-apns.mjs stops shelling out to read the key and imports the reader directly, so the key is read once, by the code that owns it. The tests cover the handoff, the missing key, the consumer's exit code, and the process table. While the consumers are waiting on App Store Connect, no process on the machine carries the key in its command line. Refs AFC-459
Closes the four review findings on the submission path: a run continued after a rejected credential, a named submission's platform was patched before its binding was verified, a refusal discarded the blockers, and an automatically discovered submission could be submitted without the requested platform. The mock binding handler now validates the body it is handed. The submit-review command sets the platform on the submission it is about to send, and it set it before it had read the binding. A submission that holds another version is left alone, and a submission that is left alone must not be mutated either, so the write happened on a submission the command was going to skip. The platform is checked before anything is written. A submission that carries another platform is never rewritten to one the operator did not ask for, and a refusal mutates nothing. A platform that was not named is this command's default, and a default must never rewrite a submission somebody else prepared for another platform. The platform is set on the candidate that is actually going to be submitted, and only after the binding has been read and made. It is then read back, so a change that was ignored or only half applied cannot slip into the call that cannot be undone. A rejected credential stops the run. The calls that follow the binding include the submit, which cannot be undone, so a 401 or 403 on the binding ends the run instead of issuing them with a credential App Store Connect has already refused. The same is true of a refusal on the submit itself, and a refusal there now prints the blockers the way a refused binding does. Refs CC-169
The owner-approved copy spec says that weekly and additional windows read weekly and additional beside the cadence token. The pair could not appear: an extra allowance carries no token of its own, and the label pipeline rewrites it into the same Other limit copy every unknown window gets, so neither the cadence nor the rendered label could identify it. The identity now travels with the window. QuotaWindowVocabulary.isExtraAllowance(providerKey:label:windowToken:) answers from the raw wire label and the provider key, and the provider-aware case is the Muse Assist one the Mac popover already treats as the extra allowance. barQualifiers(forBars:) then lets an extra allowance join the group of the one sibling that has a period, so the weekly and the extra resolve as a single cadence. With two different periods beside it the sibling is ambiguous, so the extra stays on its own and neither bar prints a qualifier. The widget carries the flag from the raw wire label and the provider key into the bar caption, because the display label erases the identity. The window's own cadence is untouched, so the seenCadenceKeys dedup cannot drop the extra bar and the plus token is preserved. The regression fixture is the real wire shape with the identifying strings sanitized, in Tests/QuotaCoreTests/WidgetPresentationVocabularyTests.swift, because the iOS target has no unit-test target. Refs CC-169
The cadence group handed the window's label to the canonicalizer, and the canonicalizer's prose matcher reads substrings. A label that merely contained a cadence word was therefore read as that window: "notweekly" grouped as a weekly bar and "notdaily" as a daily one, both before the exact-token fallback could run. The cadence field is now the only thing that may name a group, and the label resolves a group only through whole lexical tokens. The widget builds the cadence field and the display label from the same canonicalization, so the label never carried a period the cadence field had lost. Kody rule 40 (avoid substring matching for canonical limit/window labels), binding 2026-10-21.
vFp: a refused submit no longer binds the version a second time. The run records the submission it bound and stops at the refusal, because one version lives on one submission and the binding cannot be undone from the tool. u8i, u4pxt, vS4, u4qJe: the private key is read from the environment or from fleet-secrets only. asc-key.mjs lost its file reader, ios-enable-apns.mjs takes the key from process.env.ASC_KEY_P8, and ship-testflight.sh and ios-install-appstore-profiles.sh ignore a named ASC_KEY_PATH, which is not a credential source at all. vAo, audit defect 3: a child is given an allowlist of variables instead of a copy of the parent's environment, so an unrelated credential never reaches the App Store Connect client. vJZ, audit defect 2: the process table is read on an explicit readiness signal, and the check sees the environment of the key-bearing child, which is where the key lives on this platform. The comments now say what is true: the key is not in argv, not on stdout and not in a log line, and the environment of a same-user process is readable. vN_, u4qVz: the mock validates the create and patch bodies before it stores them, and a malformed body is refused with a 400 instead of throwing out of the handler. u4p6p: a create response that does not echo a platform records none, so the platform is set and read back instead of being assumed. u4qAV: the platform is set and read back before the binding, so a platform that cannot be proven leaves the version unbound and nothing to clear by hand. u4p2K: the final report tells a refusal from a run that never submitted, so the message never claims that a submission was refused when none was sent. finding 12: the mock can answer a draft with no platform, and the read-back guard is covered by tests.
Jet reviewed the qualifier work and found the rendered acceptance incomplete: the provider focus captions and the lock screen's two window captions built a caption row without the qualifier, so a bar that was meant to read "7d weekly" read "7d". The shared row file passed the qualifier and these three surfaces did not. The qualifier was an argument with a default, so a surface that rendered a window's caption could leave it out and nothing said so. It is a value now. `QuotaWindowCaption` is shared with the widget and the tests, a caption carries its qualifier, a caption built from a window carries that window's, and a caption row takes a caption. The only way to have a caption with no qualifier is to ask for one by name, so the omission is visible where it happens. The four surfaces that dropped it now print the window's caption: the provider focus per window caption, the at cap caption, and both of the lock screen's two window captions. The test starts from the bytes the backend sends. The earlier fixture was a custom struct calling the vocabulary helpers, so it could not catch a missing argument; this one decodes a real Muse Assist payload, derives the bars the way the widget derives them, and takes the qualifiers from the production resolution, then asserts the caption carries them. The iOS parser and the SwiftUI render are still in the iOS target, which has no test target, so the type is what holds those two links. This also lands the vocabulary work that was still uncommitted: an extra allowance joins the group of the one sibling that has a period, and the canonical forms the widget renders resolve their period through whole tokens, so a bar that prints "7d" is not split from the sibling that prints the same token. Five hundred and seventy-eight QuotaCore tests, green, and the iOS app and widget build.
The branch had fallen behind and the generated project file conflicted. The project file is generated from project.yml, so the conflict was resolved by regenerating it rather than by picking a side.
A shared file the iOS targets compile has to be a push path of the ship workflow and a prefix of the scheduled gate, or a change to it reaches the App Store build without a build. The caption file is compiled by the widget targets, so it is in both lists now.
The contract job runs on Linux, and the test that proves the key never reaches a command line read the environment with macOS's ps. Linux's ps has no such flag, so the probe failed with "unsupported SysV option" and the check that matters never ran. The probe now reads the environment the way the platform offers it: macOS's ps with -E, and Linux's /proc/<pid>/environ, which the owner of the process can read. All fourteen tests pass in a Linux container, which is the environment the job runs in.
The extra allowance joining the group of the one sibling that has a period, and the canonical labels naming their period, were fixed but unproved. Both rules now have a test that fails without the fix.
#257 merged to main as ad87965 and it reworked the iOS ship workflow's credential path: the tooling and the staging script now run in one step and the private key never enters a step environment. This branch still carried the pre-#257 copy of that workflow, with the inline Infisical login and a hard-coded project id, so merging it as it stood would have reverted the release fix for the iOS ship path. Merging main brings the hardened step back. No branch work was lost: the branch had no changes to the files this merge replaced.
The clients take the credential from the environment and refuse a path as a source (Kody rule 42), but the ship workflow published only ASC_KEY_PATH and the two identifiers. So every client step ran with no credential at all: ios-enable-apns.mjs resolved nothing, ios-install-appstore-profiles.sh resolved nothing, and the ship script fell through to fleet-secrets, which the hosted runner does not have. The iOS ship contract job asserted none of that, so the branch read green while the shipped workflow could not have signed anything. scripts/ios-load-asc-key.sh reads the staged mode-600 key back into the step's shell and exports it as ASC_KEY_P8, so the credential reaches the clients the way they accept it. It never enters $GITHUB_ENV, never becomes an argument and never reaches a log line. The file stays what it is: a staging detail for altool and Xcode, and the carrier between two steps of one job. The standing-testers step called asc-key.mjs exec, and asc-key.mjs has no exec subcommand since the key stopped being read from a file, so that step was dead. It now runs the client with the credential in its environment. The loader refuses an unset path, a missing file, a file that is not mode 600 and an empty file, and it names each refusal without printing the value.
The squash of PR 271 landed while the credential handoff fix was still on the branch, so main has the hardened credential load from #257 and the old standing-testers step that calls the removed asc-key.mjs exec subcommand. Merging main in picks up the squashed work and leaves the loader change as the only difference.
jaywedgeworth22
enabled auto-merge (squash)
October 21, 2026 20:21
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
GNU stat's -f does not take a permissions format, so it printed a filesystem report to stdout and then failed. The report was captured as the mode, so the loader refused a perfectly good file on every Linux runner, and the iOS ship contract job failed there. The two stat flavours are now told apart by the platform instead of being tried in turn, and the shell test asserts that the BSD form only appears on a Darwin branch, so the blind probe cannot come back. The contract test now reports the failing script's stderr, because a CI log that says only "Command failed" is a dead end.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Task description is insufficient for business logic validation. 🤔 Insufficient Task ContextI found a task linked to this PR, but it only contains minimal information (title only, no description or acceptance criteria). To perform a meaningful business rules validation, I need more details. 🔍 What I need to validate:
💡 How to improve the task context:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
Comment on lines
280
to
+288
| - name: Enable CodeCaps APNs and verify production push profile | ||
| run: node "$GITHUB_WORKSPACE/scripts/ios-enable-apns.mjs" | ||
| run: | | ||
| source scripts/ios-load-asc-key.sh | ||
| node "$GITHUB_WORKSPACE/scripts/ios-enable-apns.mjs" | ||
|
|
||
| - name: Install App Store profiles | ||
| run: bash "$GITHUB_WORKSPACE/scripts/ios-install-appstore-profiles.sh" | ||
| run: | | ||
| source scripts/ios-load-asc-key.sh | ||
| bash "$GITHUB_WORKSPACE/scripts/ios-install-appstore-profiles.sh" |
There was a problem hiding this comment.
New key-loading steps do not disable xtrace, so the PEM can be traced into the CI log
If xtrace is enabled, the expanded ASC_KEY_P8 assignment can print the unmasked private key into CI logs. Add set +o xtrace before source scripts/ios-load-asc-key.sh in every client step; keep the loader unchanged because it runs in the caller's shell.
Prompt for LLM
File .github/workflows/ios-ship.yml, lines 280-288:
New key-loading steps do not disable xtrace, so the PEM can be traced into the CI log
The three new client steps materialize the raw App Store Connect private key into a shell variable (loader: `ASC_KEY_P8="$pem"; export ASC_KEY_P8`) without first disabling shell tracing, unlike every other credential step in this same workflow (lines 241 and 247 both start with `set +o xtrace`, as do `scripts/ios-stage-asc-key.sh:3` and `scripts/install-release-secrets-tooling.sh:40`). If xtrace is ever on for the step's shell, bash echoes the expanded assignment `export ASC_KEY_P8=-----BEGIN PRIVATE KEY-----…` into the CI log, and because this key comes from Infisical rather than a repository secret the runner will not mask it. Add `set +o xtrace` as the first line of every step that sources the loader (the loader itself must not change shell options, since it is sourced into the caller's shell).
Suggested code:
- name: Enable CodeCaps APNs and verify production push profile
run: |
set +o xtrace
source scripts/ios-load-asc-key.sh
node "$GITHUB_WORKSPACE/scripts/ios-enable-apns.mjs"
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Comment on lines
315
to
+323
| set +e | ||
| # The client resolves the signing key from its environment and never | ||
| # from a path (Kody rule 42), so the staged key is handed to the | ||
| # child through the child's environment by the hardened reader. The | ||
| # key stays out of argv, out of stdout, out of $GITHUB_ENV and out | ||
| # of the log. | ||
| if [ -n "${ASC_KEY_PATH:-}" ]; then | ||
| node scripts/ios-fleet/asc-key.mjs exec "$ASC_KEY_PATH" -- \ | ||
| node scripts/ios-fleet/asc-api.mjs ensure-standing-testers "$appleid" | ||
| rc=$? | ||
| else | ||
| node scripts/ios-fleet/asc-api.mjs ensure-standing-testers "$appleid" | ||
| rc=$? | ||
| fi | ||
| # child through the child's environment by the loader. The key | ||
| # stays out of argv, out of stdout, out of $GITHUB_ENV and out of | ||
| # the log. | ||
| source scripts/ios-load-asc-key.sh | ||
| node scripts/ios-fleet/asc-api.mjs ensure-standing-testers "$appleid" | ||
| rc=$? |
There was a problem hiding this comment.
Loader failure in the tester-sync step is swallowed by set +e and misreported
Because set +e is active, a failed source scripts/ios-load-asc-key.sh is ignored, and the generic warning misdirects operators to tester sync. Capture the loader status in load_rc, warn when it is nonzero, and skip node scripts/ios-fleet/asc-api.mjs.
Prompt for LLM
File .github/workflows/ios-ship.yml, lines 315-323:
Loader failure in the tester-sync step is swallowed by set +e and misreported
The standing-testers step sources the loader while `set +e` is in effect (line 315), so a failed credential load neither aborts the step nor is reported as a load failure. The loader's `return 1` is discarded, `asc-api.mjs` then runs with no credential, and the only signal is the generic "tester sync exited N" warning, which points an operator at the tester sync instead of at the key handoff that actually failed. Capture the loader's status separately and warn on it before invoking the client.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Comment on lines
+274
to
+279
| # The clients take the credential from the environment and never from a | ||
| # path (Kody rule 42), so the staged key is handed to each of them | ||
| # through its environment. The loader reads the file the load step | ||
| # staged and exports ASC_KEY_P8 into this step's shell; the value never | ||
| # enters $GITHUB_ENV, never becomes an argument and never reaches a log | ||
| # line. |
There was a problem hiding this comment.
Header invariant "the key never enters the step environment" is now false
The documented credential boundary no longer matches the workflow, which can mislead maintainers auditing where the private key lives. Update the header to distinguish $GITHUB_ENV from each client step's shell and state that scripts/stage-asc-key.mjs stages the key for those steps to load.
Prompt for LLM
File .github/workflows/ios-ship.yml, lines 274-279:
Header invariant "the key never enters the step environment" is now false
The workflow's header contract still states "The multiline key never enters the step environment" and that "only that file's path and the two identifiers cross a step boundary", which this change makes false: the key now lives in the environment of the four client steps. The file's own documented invariant is what future maintainers will trust when auditing where the credential lives, and it now contradicts the new loader-based handoff described 250 lines below. Restate the header bullet to say the key never enters the job environment ($GITHUB_ENV) and that the steps starting a client load it into their own shell from the staged file.
Suggested code:
# * The multiline key never enters the job environment ($GITHUB_ENV).
# The tooling and scripts/stage-asc-key.mjs run in one step, so the key
# goes from the fleet's reader straight into a mode-600 file; the steps
# that start an App Store Connect client load it into their own shell.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| ASC_KEY_P8="$pem" | ||
| export ASC_KEY_P8 | ||
| unset pem |
There was a problem hiding this comment.
The helper bypasses environment-first ASC secret resolution
Directly reading $path bypasses the approved environment-first credential path, so runtime credentials can diverge from policy. Resolve ASC_KEY_P8 from the environment first, fall back to fleet-secrets get ASC_KEY_P8, and fail fast when neither source yields a value.
Also found in:
scripts/test-ios-load-asc-key.sh:47-47scripts/test-ios-load-asc-key.sh:48-48scripts/test-ios-load-asc-key.sh:69-69
Kody rule violation: Load API keys and tokens from approved sources only
Prompt for LLM
File scripts/ios-load-asc-key.sh, line 70:
The helper bypasses environment-first ASC secret resolution
The helper reads the ASC credential from a staged file instead of resolving the process environment first and Infisical through fleet-secrets second. Runtime credentials must follow the approved environment-first secret path. Read ASC_KEY_P8 from the environment when present, otherwise capture it with fleet-secrets get ASC_KEY_P8, and fail fast with the variable name if neither source resolves it.
**Also found in:**
- `scripts/test-ios-load-asc-key.sh:47-47`
- `scripts/test-ios-load-asc-key.sh:48-48`
- `scripts/test-ios-load-asc-key.sh:69-69`
Kody rule violation: [Load API keys and tokens from approved sources only](https://app.kodus.io/settings/code-review/global/kody-rules/543a7760-6c23-4656-b6a4-b151c40a46a6?teamId=69360e3a-6b41-45d2-9e49-fde78fe49457)
Suggested code:
if [ -n "${ASC_KEY_P8:-}" ]; then
pem="$ASC_KEY_P8"
else
pem="$(fleet-secrets get ASC_KEY_P8)"
fi
if [ -z "${pem//[[:space:]]/}" ]; then
echo "error: ASC_KEY_P8 is not set and could not be resolved." >&2
return 1
fi
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| ASC_KEY_P8="$pem" | ||
| export ASC_KEY_P8 | ||
| unset pem |
There was a problem hiding this comment.
The helper reads the ASC secret from an arbitrary file
Permitting credentials from an unapproved local source weakens the secret-source boundary. Replace the file read with environment-first resolution of ASC_KEY_P8, then fleet-secrets get ASC_KEY_P8, and fail fast if neither source returns a value.
Also found in:
scripts/test-ios-load-asc-key.sh:47-47scripts/test-ios-load-asc-key.sh:48-48scripts/test-ios-load-asc-key.sh:69-69
Kody rule violation: Never mint provider API keys or expose secret values — secret hygiene is binding for every agent
Prompt for LLM
File scripts/ios-load-asc-key.sh, line 70:
The helper reads the ASC secret from an arbitrary file
The helper retrieves the ASC credential directly from an arbitrary local file. Secrets may only come from the process environment, fleet-secrets get, or bootstrap-only Keychain credentials. Resolve ASC_KEY_P8 from the environment first, then use fleet-secrets get ASC_KEY_P8 as the Infisical-backed fallback, and remove the direct file read.
**Also found in:**
- `scripts/test-ios-load-asc-key.sh:47-47`
- `scripts/test-ios-load-asc-key.sh:48-48`
- `scripts/test-ios-load-asc-key.sh:69-69`
Kody rule violation: [Never mint provider API keys or expose secret values — secret hygiene is binding for every agent](https://app.kodus.io/settings/code-review/global/kody-rules/3f722a39-4c80-41d0-b1fb-0151688e9f32?teamId=69360e3a-6b41-45d2-9e49-fde78fe49457)
Suggested code:
if [ -n "${ASC_KEY_P8:-}" ]; then
pem="$ASC_KEY_P8"
else
pem="$(fleet-secrets get ASC_KEY_P8)"
fi
if [ -z "${pem//[[:space:]]/}" ]; then
echo "error: ASC_KEY_P8 is not set and could not be resolved." >&2
return 1
fi
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Comment on lines
318
to
+323
| test("scheduled gate and key handoff shell tests pass", () => { | ||
| for (const script of ["scripts/ios-fleet/test-scheduled-ship-gate.sh", "scripts/test-ios-stage-asc-key.sh"]) { | ||
| for (const script of [ | ||
| "scripts/ios-fleet/test-scheduled-ship-gate.sh", | ||
| "scripts/test-ios-stage-asc-key.sh", | ||
| "scripts/test-ios-load-asc-key.sh", | ||
| ]) { |
There was a problem hiding this comment.
New shell test is not a trigger path for the contract job that runs it
Changes to scripts/test-ios-load-asc-key.sh alone do not trigger the contract job, so its assertions can break without running. Add scripts/test-ios-load-asc-key.sh to both on.pull_request.paths and on.push.paths in .github/workflows/ios-ship-contract.yml.
Prompt for LLM
File scripts/ios-ship-workflow.test.mjs, lines 318-323:
New shell test is not a trigger path for the contract job that runs it
The new shell test `scripts/test-ios-load-asc-key.sh` is executed by the iOS ship contract job but is not one of that job's `paths:` filters, so a change to it alone does not trigger the job that runs it. `.github/workflows/ios-ship-contract.yml` lists `scripts/test-ios-stage-asc-key.sh` explicitly (lines 20 and 30) but not the new file; `scripts/ios-*` and `scripts/stage-asc-key*` do not match a `scripts/test-…` path, so the contract assertions over the loader can be edited or broken without any CI run, which is exactly the failure mode the contract job exists to prevent. Add `- 'scripts/test-ios-load-asc-key.sh'` to both `on.pull_request.paths` and `on.push.paths` in `.github/workflows/ios-ship-contract.yml`.
Suggested code:
# .github/workflows/ios-ship-contract.yml — in BOTH on.pull_request.paths and on.push.paths:
- 'scripts/test-ios-stage-asc-key.sh'
- 'scripts/test-ios-load-asc-key.sh'
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| ( | ||
| export ASC_KEY_PATH="$key_path" | ||
| source "$repo_root/scripts/ios-load-asc-key.sh" > /dev/null 2>&1 | ||
| node -e 'process.stdout.write((process.env.ASC_KEY_P8 || "") + "\n")' | cmp - "$tmp_dir/expected" |
There was a problem hiding this comment.
The resolved credential is emitted to stdout
Perform the comparison inside Node and return only an exit code, without printing the secret or its digest.
Also found in:
scripts/test-ios-load-asc-key.sh:47-47scripts/test-ios-load-asc-key.sh:48-48
Kody rule violation: Never expose secrets or private infrastructure values; reuse existing fleet env vars
Prompt for LLM
File scripts/test-ios-load-asc-key.sh, line 69:
The resolved credential is emitted to stdout
process.stdout.write expands the secret-bearing ASC_KEY_P8 environment variable into output. Output statements must not reveal a resolved secret value. Perform the comparison inside Node and return only a success or failure exit code, without writing the key or its digest.
**Also found in:**
- `scripts/test-ios-load-asc-key.sh:47-47`
- `scripts/test-ios-load-asc-key.sh:48-48`
Kody rule violation: [Never expose secrets or private infrastructure values; reuse existing fleet env vars](https://app.kodus.io/settings/code-review/global/kody-rules/1347af05-4470-44b7-bcdc-4377eeabb35e?teamId=69360e3a-6b41-45d2-9e49-fde78fe49457)
Suggested code:
node -e 'const fs = require("fs"), crypto = require("crypto"); const expected = fs.readFileSync(process.argv[1], "utf8").trim(); const actual = crypto.createHash("sha256").update(process.env.ASC_KEY_P8 || "").digest("hex"); process.exit(actual === expected ? 0 : 1)' "$tmp_dir/expected"
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
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
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.
Refs CC-169
Refs CC-177
Summary
Refactors the iOS App Store Connect workflow to hand the staged private key to clients through the
ASC_KEY_P8environment variable. The staged file remains an internal transport artifact, but clients no longer receive a credential path or use the removedasc-key.mjs execcommand.Changes
scripts/ios-load-asc-key.sh, a sourceable helper that:ASC_KEY_PATHto reference an existing regular file.0600.statbehavior on macOS and Linux.ASC_KEY_P8in the current shell without intentionally printing it or writing it toGITHUB_ENV.asc-key.mjs exec.scripts/ios-load-asc-key.shto the workflow’s push-path triggers.scripts/ios-ship-workflow.test.mjsto verify that every ASC client step sources the loader, that the removed command is not restored, and that the loader exports the credential without directly logging it.scripts/test-ios-load-asc-key.shwith synthetic coverage for successful loading, child-process environment propagation, alternate key files, missing or unset paths, insecure permissions, empty content, platform-specific mode checks, and avoidance ofGITHUB_ENVwrites.Review Caveats
ASC_KEY_P8assignment and PEM contents in CI logs.ASC_KEY_PATHeven whenASC_KEY_P8is already available and accepts any file that satisfies the basic existence, permission, and content checks. This does not provide environment-first credential resolution or validate the staged file’s provenance.set +eand captures only the following client’s exit code, so a loader failure can be swallowed or misreported as a tester synchronization failure.scripts/test-ios-load-asc-key.shis not currently a trigger path for the contract job that runs it.