Separate host admission from projection selection - #225
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.99
The new projection model is internally consistent for fresh repositories, but two strict version changes make the stated existing-repository migration path unreachable. Model-level verification is not recommended before merge; deterministic cross-version fixtures using the actual base binary should first cover configuration, manifest, artifact, and ownership migration.
Findings without inline diff anchors
[P1] Provide a governed bridge from configuration schema 3
Invariant: an installed repository must retain an admitted transition that can establish the next version's required configuration and control-bundle files. A base-revision installation has schema-3 project.json, runtime.json, and host-skills.json; the old runtime rejects a schema-4 mutation candidate, while the head runtime rejects the durable schema-3 file here before configuration.mutate or installation.update can run. The new source-bundle check also requires host-projections.json before the update that would create it. Thus the documented migration sequence cannot start, leaving existing repositories unable to upgrade without an out-of-band rewrite or reinstall. A regression test should install with the actual base binary, then use the head candidate to govern the schema change and update successfully, including conversion of the legacy manifest.
Confidence: 0.99
Location: boatstack/internal/softwaredelivery/protocol/config.go:159-160 (RIGHT)
[P1] Migrate existing Flow projection ownership records
Invariant: prior kernel-owned provenance must remain readable until it has safely authorized its replacement. A Flow compiled by the base revision leaves a schema-1 ownership record containing generated_skills; head flow compile loads that record into the new schema-2 structure, and the strict decoder/schema check rejects it before any projection or artifact can be replaced. Since head also rejects the old compiler-5 artifact, recompilation—the intended migration path—is deterministically blocked for every previously compiled Flow. The observable result is that existing Flows cannot be recompiled or resumed without deleting authority metadata manually. A regression test should compile a Flow with the actual base binary, then compile it with head and verify atomic conversion to schema 2 plus safe retirement of the old generated files.
Confidence: 0.99
Location: boatstack/internal/runtime/flow_ownership.go:207-218 (RIGHT)
|
Review cycle 1 disposition (exact head Both reported P1s require compatibility readers/migration for schema-3 configuration, No code change was made for these findings because it would violate the breaking boundary. Verification after disposition:
The lack of in-place v3/v1 migration is a deliberate release boundary and should remain visible in review rather than being silently reintroduced as compatibility behavior. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces a blocked base-to-head upgrade, a shared-projection TOCTOU race, and two completeness regressions. Model-level verification is recommended before merge, focused on whether projection-selection changes can interleave with Flow publication or retirement into any other manifest/ownership mismatch.
| manifestPath := filepath.Join(repository, ".boatstack", "host-projections.json") | ||
| manifestRaw, manifestExists, _, err := readAllIfExists(manifestPath) |
There was a problem hiding this comment.
[P1] Preserve a governed upgrade path for legacy projection manifests
Invariant: a repository installed by the base revision must be able to reach the new admitted projection format through the documented governed update path. A base installation has .boatstack/host-skills.json and generated boatstack-update files; the head reads only .boatstack/host-projections.json, treats the existing generated files as unmanaged, and configuration.mutate refuses with an unmanaged collision. The schema-3 configuration is also rejected before update admission, so there is no head transition that can migrate the legacy ownership. Existing repositories cannot update without manually deleting or rewriting kernel-owned state. A regression test should initialize with the base binary, then use the head binary to govern a schema-4 projection change and update successfully while preserving exact legacy ownership.
Confidence: 0.98
| if hostprojection.IsSharedCheckoutPath(relative) { | ||
| referenced, referenceErr := boatstackruntime.SharedFlowProjectionReferenced(repository, relative, prior.Files[relative]) |
There was a problem hiding this comment.
[P1] Serialize shared-projection reference checks with configuration mutation
Invariant: a committed projection-selection transition must not remove a file referenced by a concurrently committed Flow ownership record. configuration.mutate can observe no Flow reference here and prepare deletion of a shared Cursor/Gemini .gitattributes; before its mutations execute, flow compile can acquire the projection lease and commit that file plus a new ownership record. The configuration transaction does not acquire that lease, so it then deletes the now-referenced file and commits successfully, leaving the Flow artifact and ownership record bound to an absent output; subsequent checks fail, and recompilation cannot recreate it because provenance expects an existing prior hash. This race is introduced by sharing these paths across maintenance and Flow projections. A regression test should pause configuration mutation after this reference check, compile the first Flow owner, resume the mutation, and assert that either the mutation retries/refuses or the shared file remains.
Confidence: 0.95
| paths, err := hostprojection.FlowPaths(projection, slug) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
[P2] Encode all valid control-program IDs into projection slugs
Invariant: every Control Program accepted by the IR ABI must remain projectable for a selected supported host. The ABI permits . and _ in program and entry IDs, but flowSkillSlug leaves those characters unchanged and FlowPaths now accepts only alphanumeric/hyphen slugs. Thus a valid program such as product.delivery with entry run, or program product with entry run_now, compiles normally but GenerateProjections deterministically returns FLOW_PROJECTION_PATH_INVALID, preventing boatstack flow compile. The new path validator introduces this rejection; previously these IDs produced projections. Add a regression test projecting valid dot/underscore IDs for each host and assert collision-free encoded paths are generated.
Confidence: 0.99
| size, parseErr := strconv.ParseInt(fields[2], 10, 64) | ||
| if parseErr != nil || size < 0 || size > 64<<20 { | ||
| return fmt.Errorf("CONTROL_BUNDLE_STALE: revision %s has invalid size for %s", revision, file.Path) |
There was a problem hiding this comment.
[P2] Do not reject exact committed bundle files solely above 64 MiB
Invariant: any control bundle accepted at the repository root must also verify at an exact revision containing identical bytes. Snapshot construction and root verification impose no 64 MiB limit, so a valid large dependency lock or other bound file can be hashed and committed; workspace.cut then calls revision verification, which rejects that identical blob as an invalid size. This patch adds the asymmetric cap and makes such repositories unable to create a managed workspace. A regression test should commit a 64 MiB-plus-one bound file, verify the root snapshot, and require VerifyControlBundleRevision to accept the exact commit (or enforce the same documented limit when constructing the snapshot).
Confidence: 0.98
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch blocks the advertised base-to-head upgrade path and introduces a control-bundle verifier that rejects snapshots accepted by admission. Model-level verification is recommended after these fixes, especially for progress across projection-selection changes and shared projection retirement.
|
Review cycle 1 settled at head d6f5f35 (reviewed head b22f564).
Public verification: TestProjectConfigurationIsStrictAndVersioned; TestControlBundleSizeLimitIsConsistentAcrossAdmissionAndVerification; full internal/runtime tests; full internal/runtime race tests; dependent boatstack-helper control-bundle tests; go vet ./internal/runtime; git diff --check. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new projection behavior is internally coherent for fresh schema-4 repositories, but the strict, simultaneous persistent-format changes make the advertised existing-repository update path unreachable. Model-level verification is not recommended before merge; this is a deterministic compatibility failure best covered by a real base-to-head upgrade test.
Findings without inline diff anchors
[P1] Schema 4 leaves existing installations with no governed upgrade path
Invariant: an installed repository must remain able to reach its update transition through kernel-owned state changes. Minimal sequence: initialize with the base revision (schema-3 project config, schema-1 host-skill manifest, schema-3 Flow artifacts) → run the head installer in update mode → the head runtime loads the current control bundle → Validate rejects schema 3 before resolution or effects. The base runtime also rejects a schema-4 configuration candidate, while the head subsequently rejects the old manifest, artifact, and ownership schemas, so retrying or following the documented add/configure/recompile/update sequence cannot progress. This patch introduces all of these strict schema bumps without a transition that migrates them, making existing repositories unupgradable except through out-of-band deletion/reconstruction of kernel-managed state. The smallest regression test should initialize a repository using an actual base-revision binary, then run the head update path and verify a governed migration completes and the head runtime can load the resulting repository.
Confidence: 0.98
Location: boatstack/internal/softwaredelivery/protocol/config.go:159-160 (RIGHT)
|
Review cycle 1 (configured three-cycle run) settled at exact head d6f5f35. The sole P1 asks for schema-3 and legacy-state migration. It is unsupported by the accepted flag-day alpha contract, and the user explicitly confirmed that breaking changes do not need compatibility repair. No compatibility reader, alias, or migration shim was added. The exact-head refusing test TestProjectConfigurationIsStrictAndVersioned passes; all 18 exact-head checks pass. There were no inline comments in this batch. |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The new projection model is internally well-bound for fresh repositories, but the patch lacks a reachable governed migration from the stated base and introduces an unlocked ownership/deletion race. Model-level verification is not recommended before merge; real base/head migration and deterministic concurrency tests are the appropriate closure oracles.
Findings without inline diff anchors
[P1] Provide a governed upgrade path from schema 3
Invariant: an installed base-revision repository must be able to reach the new admitted formats without first mutating kernel-owned state outside Boatstack. A base installation has schema-3 project configuration, artifact/ownership schema 3/1, and .boatstack/host-skills.json; invoking the head update rejects the current config here before resolution. The old runtime also rejects a schema-4 candidate, while the head compiler rejects old ownership records and the new projection writer treats legacy generated files as unmanaged collisions. Thus the documented add-projections → recompile → update sequence is unreachable and existing repositories cannot upgrade without manually deleting or rewriting governed files. Add an actual base-to-head fixture that installs with the base binary and upgrades with the head binary, asserting committed configuration and installation receipts plus migrated manifests/ownership.
Confidence: 0.99
Location: boatstack/internal/softwaredelivery/protocol/config.go:159-160 (RIGHT)
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces an upgrade dead end for existing Flow ownership records and rejects valid ControlProgram identities during projection generation. Model-level verification is recommended before merge, particularly to check whether any mixed ownership/manifest version or shared-file retirement interleaving can become blocking or lose its recovery path.
|
Review cycle 2 settled at exact head
Public verification: |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.97
The patch can overwrite unowned shared host metadata and can commit an installation target whose Flow artifacts contradict its configuration. Model-level verification is recommended before merge: verify shared-owner retirement across maintenance/Flow selection changes and that every initialization target is semantically reconstructible.
Findings without inline diff anchors
[P1] Validate initialization artifacts against the candidate projection selection
Invariant: a successfully committed installation target must be semantically reconstructible from its project configuration and Flow artifacts. Start with fresh durable state but an existing valid project using projection set A and Flow artifacts compiled for A, then run installation.initialize with candidate config B. The source snapshot validates against A; this code replaces only project and maintenance-projection files, retains the A-bound artifacts, and commits that mixed snapshot as the target. Hash verification succeeds, but the next normal bundle build deterministically fails with FLOW_PROJECTION_SELECTION_STALE. This mismatch is introduced because artifacts now bind projection selection while initialization target construction does not revalidate them. The result is a successful installation receipt followed by a repository that cannot operate until an out-of-band recompile. A regression test should initialize an A-compiled repository with candidate B and require refusal with zero effects. The target must either validate every artifact against B or reject the selection change before applying.
Confidence: 0.96
Location: boatstack/cmd/boatstack-helper/control_bundle.go:328-332 (RIGHT)
|
Review cycle 3 settled at exact head
Public verification: full Flow and ControlProgram suites, focused projection/host-path suites, race-enabled Flow projection tests, vet, |
|
Post-cycle-3 P1 settlement at exact head
Verification: full |
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces a deterministic self-lock in a reachable recovery transition and leaves the stated existing-repository upgrade procedure with no executable bridge across its persisted schema changes. Model-level verification is recommended for all recovery transitions that acquire the new projection lease, particularly to confirm each lease is acquired exactly once and retains a progress path.
Findings without inline diff anchors
[P1] Provide a reachable upgrade path from the persisted schema-3 formats
Invariant: an advertised upgrade sequence must have an authorized operation that can read the old durable formats and commit their replacements. Starting with a repository produced by the base revision (project.json schema 3, artifact schema 3, ownership schema 1), the head candidate rejects the current config while building the source control bundle, so installation.update cannot run. The old runtime also rejects a schema-4 configuration.mutate; even after an unmanaged config edit, strict schema-2 ownership decoding prevents the documented Flow recompilation and artifact schema 3 is rejected. Thus existing repositories cannot perform the release-note sequence or obtain an update receipt without manually deleting/resetting control state. This is introduced by bumping all formats while retaining only current-version decoders. The regression should create the repository with the actual base binary, then exercise the documented configuration change, Flow recompilation, and installation update with the head binary while asserting preserved state and a committed update receipt.
Confidence: 0.96
Location: boatstack/internal/softwaredelivery/protocol/config.go:159-160 (RIGHT)
| case "configuration.initialize", "configuration.mutate", "installation.initialize", "installation.update", "installation.reconcile-update": | ||
| return true |
There was a problem hiding this comment.
[P1] Include configuration reconciliation in maintenance lease ownership
Invariant: a prescribed recovery transition must not deterministically contend with a lease already held by its own invocation. A repository Flow can expose configuration.reconcile; when configuration recovery is required, apply first acquires the Flow projection lease here because this transition is absent from the exemption list. The engine then acquires its configuration resource lock, whose patched locker attempts to acquire the same non-reentrant projection lease and returns FLOW_PROJECTION_BUSY. State remains unchanged and every retry repeats the refusal, making configuration recovery unreachable for that Flow. Add a production-path regression test that prescribes and applies configuration.reconcile through a repository Flow and requires a committed receipt.
Confidence: 0.99
1 participant
Summary
hostsfrom explicit repositoryprojectionsin strict project configuration schema 4internal/hostprojectionmodel, canonical SHA-256 selection fingerprint, four host-native layouts, and strict path validationGenerateProjections,GeneratedProjections,ProjectedHostProjectionFiles, and.boatstack/host-projections.jsonhostsBreaking impact
Existing repositories must add an explicit
projectionsarray, apply it throughconfiguration.mutate, compile and check every Flow while product work remains suspended, then run the normal installation update before resuming. Missing/null, duplicate, unsupported, and non-subset selections fail closed; explicit[]is valid.Verification
npm run test:flow-sdknpm run docs:checkpython3 .github/scripts/run_go_tests.py --jobs 2— 533/533 tests passedpython3 -m unittest discover -s .github/tests -p 'test_*.py' -v— 55 passed, 1 expected failurego test -race ./...go vet ./...go build ./...GOOS=windows GOARCH=amd64 go build ./...bash -n install.shgit diff --checkThe default ten-worker local Go controller was also exercised; four existing subprocess deadline tests exceeded their 30-second bound under host contention. The complete suite passed at two isolated workers, and those four tests passed together immediately in 1.331 seconds.
Consumer proof
An independent clean
bigboateng/boatstack-testclone at base1d678acb5502d5c1dd7b8fe8c1366cde87d637c3used the exact candidate helper and locally packed TypeScript packages:hosts=cli,codex,cursor,projections=codexproved Cursor runtime admission without Cursor generated files and reached a fresh Codex Product Delivery delegation boundary.ProgramFingerprint, changed configuration/control-bundle fingerprints, and passed Flow checks.Consumer worktree:
/private/tmp/boatstack-test-host-projection-selection-final(left clean; no consumer PR).Locus
Locus validated a hash-bound model of commit
6a1609e121a53e86a3bc63ae52563bcf54a5a98cas faithful and found no modeled forbidden path (res-719fc5602029edb5b8ed6b1ade0e14988c39f48f9e2361e521d219e7ac0c29b1). The bounded inventory classifies 12 modeled admission/generation/publication/retirement sites and three non-mutating exclusions;event-completenessis fully discharged with path/line evidence and refusing tests.The system claim remains advisory: Locus additionally requires Git-revision lineage for every evidence reference, while the inventory receipt is intentionally scratch-held. The verified-target frontier therefore reports
blockedwith no remaining implementation action. One oversized Observatory spool was retained locally after a413; subsequent final runs archived successfully.