carlos / platform Public

Reviewed the plan at 7a8cf520 against the working tree and spec. This was a static review; the proposed code and tests were not executed. plan:L below refers to that plan’s line numbers.

1. P1 — Task 2, Step 3: missing import prevents compilation. plan:876 introduces status.InstanceID(), but internal/host/edgesfile.go:3–20 does not import internal/status. The import in edgesfile_test.go does not apply to another file. Fix: explicitly add github.com/carlosframework/platform/internal/status to edgesfile.go.

1. P1 — Task 1, Steps 1 and 3: the storage key is not bound to the verified box. Load reads Key(box) but never checks rec.Box == box (plan:378–398). An intact, valid record for A stored under control/boxes/B.json therefore passes verification as A (plan:470–471). Review Focus 5’s test changes the record’s Box field instead of exercising this substitution (plan:197–203). Fix: reject a storage-key/record mismatch and test loading A’s unchanged record from B’s key.

1. P1 — Task 1, Step 4 / Task 3, Step 3: a supported but wrong region is accepted. Verification selects a certificate using the document’s own region, with no expected-region comparison (plan:453–461). Eligibility then selects by unsigned placement metadata without comparing the verified region (plan:1254–1274). A signed US identity can qualify through an edges file advertising the Irish placement, contrary to spec lines 193–196. Fix: require the signed region to match the expected AWS region; test a mismatch where both regions have certificates.

1. P1 — Task 6, Step 3: readStable can follow ancestor symlinks or block opening a substituted FIFO. Lstat followed by ordinary os.Open (plan:2551–2579) does not secure the path. Replacing a parent directory with an external symlink before the read lets both inode checks succeed on the external file. Replacing the final file with a FIFO between inspection and opening can block before f.Stat. Task 7 reaches this through queued paths at plan:3144. Fix: use root-relative traversal that refuses symlink components and a nonblocking, no-follow final open. Add deterministic ancestor and FIFO substitution tests.

1. P1 — Task 5, Step 5: restore can write or delete outside its destination. CleanPath checks spelling, but filepath.Join and os.Remove follow ancestor symlinks (plan:2219–2226), as do the parent-directory operations in writeAtomic (plan:2271–2274). With files/sub -> outside, entry sub/x modifies outside/x. Fix: constrain all restore operations, including tombstones and temporary files, to an opened destination root. Test both writes and deletes through an external symlink.

1. P1 — Task 11, Step 2: the queue policy does not enforce exclusive termination-event provenance. The statement at plan:4655–4662 only grants the EventBridge rule access. A same-account principal with an identity-based sqs:SendMessage grant can still submit a forged termination event, whose JSON fields Task 9 accepts. AWS documents that these grants combine unless explicitly denied. SQS policy evaluation. Fix: explicitly deny SendMessage when the source ARN is missing or differs from the rule. Test both the legitimate rule and an otherwise IAM-authorized sender.

1. P2 — Task 5, Step 5: directory-to-file transitions cannot restore. Publish a/b, then replace directory a with file a. The resulting manifest has live a and tombstoned a/b. Restore sorts paths (plan:2216), writes a, then fails removing a/b with ENOTDIR (plan:2222–2226). Restoring over an earlier copy also fails when renaming a file over the existing directory. Fix: handle deletions and file/directory transitions explicitly. Test both transition directions against empty and previously restored destinations.

1. P2 — Task 7, Step 9: overflow does not repair missing directory watches. IN_Q_OVERFLOW requests a scan without marking coverage degraded (plan:3434–3436), while run repairs watches only when degraded (plan:3093–3098). If overflow loses a directory-creation event, the scan publishes its current files but leaves the directory unwatched; later changes wait ten minutes. Fix: mark coverage uncertain, rebuild recursive watches, and retain polling until repair succeeds. Test a lost directory-creation event followed by another write inside that directory.

1. P2 — Task 7, Steps 6 and 9: watcher failure can disable fallback polling. A read error exits the reader and closes its channel (plan:3403–3411). run ignores the receive’s ok value (plan:3076), producing a busy loop. Repair can then clear degradation after adding watches even though no reader remains (plan:3332–3336). Fix: detect channel closure and recreate the watcher or stay in polling mode. Test continued publication after the event channel closes.

1. P2 — Task 2, Step 3: headroom never refreshes solely on the five-minute tick. The new comparison deliberately removes headroom (plan:853–871), but the existing timed path still calls the same gated function without forcing a write (internal/host/edgesfile.go:692–695). If nothing else changes, headroom remains stale indefinitely, contrary to plan:642. Fix: distinguish a periodic refresh from a change-triggered pass. Test that headroom-only changes skip ordinary passes but publish on a timed refresh.

1. P2 — Task 8, Step 6: importing default replication leaves the destination’s previous class. ImportWake skips SetReplication when the source value is empty (plan:3895–3898). Importing default state onto a destination set to daily therefore retains daily. Empty is explicitly valid and supported by the setter (internal/registry/registry.go:415–420, 1259–1278). Fix: apply the empty value too; test an empty source class against a nonempty destination.

1. P2 — Task 3, Steps 1 and 3: chooseBox implements a different spread rule. Spec lines 266–269 prefer a box with no instance of the app, then use total claimed-host count. The implementation instead minimizes app-instance count before total load (plan:1299–1306). Its test deliberately picks a box with five total hosts over one with two when both already run the app (plan:1204–1206). Fix: compare whether the app count is zero, then total hosts, then box ID; update that test.

1. P2 — Task 6, Step 3: directory exclusions can tombstone previously published children. The walk respects skip("cache", true), but the deletion pass checks only skip("cache/x", false) (plan:2627–2630, 2666–2670). With the plan’s callback, an existing cache/x entry becomes a tombstone. Individual-file exclusions are preserved, so the two exclusion forms behave differently. Fix: retain excluded directory prefixes during reconciliation and test exclusion after the child has already been published.

1. P2 — Task 7, Step 4: startup scanning masks event and polling test failures. startRun returns before its mandatory initial scan finishes (plan:2878–2883, 3070). The deadline, overflow and polling tests immediately create their files (plan:2908–2915, 2936–2965), so the initial scan can satisfy their assertions with the tested mechanism disabled. Fix: synchronize on completion of the initial scan before making changes; assert the publication timing and disable unrelated scan paths.

1. P2 — Task 6, Step 1: Review Focus 2 and 3 are only partly tested. The nonregular-file test covers static symlinks, but no FIFO, socket, device or substitution race (plan:2430–2453). The oversized-file test checks the final skipped result, which would also pass if the implementation read the entire file before rejecting it (plan:2457–2480). Fix: add applicable special-file cases and deterministic races, plus an instrumented read boundary that proves oversized files are rejected without reading their contents.

1. P2 — Task 8, Step 1: the all-or-nothing test never reaches the transaction. Its wrong-host row (plan:3575–3579) is rejected before Begin (plan:3620–3625). The test would pass with nontransactional deletion and insertion. Fix: use duplicate names belonging to the correct host so insertion fails after deletion and one successful insert; assert that the original rows survive.

1. P2 — Task 9, Step 1: the unverified-identity test passes for the wrong reason. FromDescribe(..., "", "") returns Found:false immediately (plan:4065–4068, 4193–4196). Removing Verify’s empty-identity guard would still leave the test passing on absence. Fix: construct a Found:true, terminated, describe observation with matching empty identity coordinates and verify that the identity guard rejects it.

1. P2 — Task 4, Step 4: required conflict diagnostics are absent. The plan promises a once-per-refresh diagnostic for equal positive stamps (plan:1348), and spec lines 322–326 require an alert for conflicting unstamped entries. Neither the fold replacement nor dialBeats implements reporting (plan:1497–1529). Fix: add refresh-scoped diagnostics and tests that preserve the deterministic winner while checking the warning.

1. P3 — Task 5, Step 6: the stated storage protection is deferred elsewhere. plan:2303 says Task 11 enforces create-only content objects, but Task 11 protects only control/boxes/; the files-prefix policy is explicitly deferred to 1b at plan:4877. Fix: correct the statement to describe the deferred enforcement and its prerequisite before enabling the publisher.

Verdict: not ready.