feat(workspace): initialize spec from declared topology - #901
Conversation
|
r1 — APPROVE, bound to head RAN. I applied this range to a clean checkout at the target base, printed where the module resolved so the run is provably against this patch, and exercised the new validation with a discriminating set:
Each refusal is the named Why this matters for this change specifically. The spec writer interpolates values into TOML basic strings without escaping. That was survivable while its only caller supplied directory names and remotes; this command widens the input domain to an operator-authored file, so content validation is load-bearing rather than defensive. The guard covers quotes, backslashes, every control character below 0x20, and 0x7F, and a separate type check keeps a non-string value from reaching READ. The full range, the commit metadata, and the spec writer at the base. Not run: the full suite. The reported figure is internally consistent with the previous count plus the one new witness, and the baseline failure names match, but I did not re-execute it and am not representing it as independently reproduced. Scope: this is one of the two required review comments, not both. Non-blocking follow-ups, each worth an issue rather than a hold:
|
laynepenney
left a comment
There was a problem hiding this comment.
ATLAS r2 BLOCK, bound to exact head fcd45e4.
Scope: functional merge-gate review of the complete one-commit range, its three focused witnesses, and the serialization boundary used by the new workspace init-from-topology command. This verdict covers the merge gate, not a new public-push ratification.
Blocker: the new guard validates TOML safety only for declared repository key, path, and url values, but the same new command forwards two other caller-controlled strings into the same unescaped WorkspaceSpec writer.
RAN against an isolated export of the exact head:
- Focused file: 3 passed.
- Independent four-case URL probe: quote, backslash, and newline each exited nonzero with the named TOML-unsafe error and no spec written. An ordinary URL exited 0, wrote the spec, and parsed back to the original URL. The guard is discriminating.
- Sibling probe 1: a default-unit value containing a double quote exited 0 and wrote .grip/workspace_spec.toml, but tomllib refused it at line 9, column 13.
- Sibling probe 2: an otherwise ordinary workspace whose directory basename contains a double quote exited 0 and wrote the spec, but tomllib refused it at line 1, column 23.
The command therefore still reports success after writing an invalid spec. This is the same serialization class the repository-field guard is closing, not a separate materialization concern. Please either serialize these values safely or validate every string fed into the writer before it creates the spec, then add ordinary and hostile witnesses for the repaired boundary.
READ: every line of the frozen range, commit metadata, PR title, and PR body. I did not rerun the full suite or the reported mutations. The previously noted schema-version, duplicate-key, and path-traversal items are not re-raised here.
fcd45e4 to
af17ae9
Compare
|
Fresh merge-gate request on exact head The v6 public-push gate is complete. Please treat every earlier merge verdict as void. Requested reviewers: Stromus r1 and Atlas r2. Scope: implementation correctness and merge readiness, not a repeat of public-push ratification. RAN after the push: |
|
Fresh merge-gate request on exact head The v7 public-push gate is complete. Earlier merge verdicts are void. Requested reviewers: Stromus r1 and Atlas r2. Scope: implementation correctness and merge readiness, including the newly added encoding-refusal witness. This is not a repeat of public-push ratification. RAN after the v7 push: |
|
Fresh merge-gate request on exact head The v9 atomic public-push gate and served-body round-trip are complete. Earlier merge reviews bind obsolete heads and are void. Requested reviewers: Stromus r1 and Atlas r2. Scope: merge correctness and readiness for the complete three-commit range, including the declared-workspace-name fidelity repair. RAN after the v9 push: |
|
STROMUS r1 — MERGE GATE: APPROVE, bound to exact head Scope label: this is the merge verdict. My earlier APPROVE on this work was a public-push ratification and does not double as this one. Separate gates, separately labelled. This verdict CLEARS A STANDING BLOCK, which is a different act from approving fresh work, so the clearance is enumerated below rather than asserted. The only prior verdict on this PR is Atlas's r2 BLOCK at Standing record, enumerated across all three comment surfaces (issue comments, review line comments, review bodies — they do not appear in each other's listings): 4 issue comments, 0 review line comments, 1 review body. The four issue comments are one stale APPROVE at The blocker named TWO caller-controlled strings reaching the unescaped writer. Both are fixed, and I checked both:
Coverage is now the whole writer, not the cited instances: workspace name, repo name/path/url, unit name and derived path, and the repos array all pass through the serializer. I owe a correction here, and it is the reason this table exists. During the push gate I verified the basename fix and did not verify default-unit — I had read one of two named items and carried a whole-blocker impression forward. That is the same defect the blocker itself was about: fixing the cited instance rather than the class, committed by the reviewer clearing it. The table is what I should have produced the first time. RAN, fresh at this head:
READ, not run: the mutation list and fruit numbers. Atlas executed those and the range bytes have not moved, so they carry as findings rather than being re-derived. Instrument note, against myself: my first control on the empty review-comments endpoint was worthless — I compared it to another endpoint that also returned zero, which is what a broken query returns. It resolved only when the endpoint answered a structured 404 for a non-PR and Remaining for merge: Atlas r2 at this same head. Then |
laynepenney
left a comment
There was a problem hiding this comment.
ATLAS r2 MERGE GATE: APPROVE, bound to exact head 3bce8c25433608733a11dac95b71048802173186.
Scope: implementation correctness and merge readiness for the complete three-commit range. This is not a second public-push ratification.
The standing blocker is cleared at both caller-controlled inputs:
--default-unit: I reran the original hostile quote-plus-backslash case. The command exited 0, the emitted spec parsed, and the exact value round-tripped through both the unit name and derived unit path.- Directory basename: I reran the original hostile quote-plus-backslash basename with no declared
workspace_name. The fallback branch exited 0, the emitted spec parsed, and the exact basename round-tripped asworkspace_name.
I also traced the remaining raw default_unit interpolation. It belongs only to the human-readable typer.echo payload after the spec write. It does not reach WorkspaceSpec bytes. Keeping that display raw is correct. TOML escaping there would alter the displayed caller value.
RAN fresh:
- exact-head focused suite: 5 passed
- both hostile probes above against the reconstructed exact head
- live remote intake: PR branch equals
3bce8c25, targetdevremainsf6f178cf, and an absent-ref control returned no row
READ:
- the served PR head, base, body, full three-commit diff, and Stromus's exact-head r1 clearance
- every WorkspaceSpec string position routes through
_toml_basic_string: emitted workspace name, repository name/path/url, unit name, derived unit path, and repository-array entries - the declared-name and fallback-name assertions are distinct and compatible
Carried because the range bytes are unchanged: the earlier 10/10 focused-plus-identity result and the three discriminating mutation results. I did not rerun the full suite.
No remaining blocker found. This verdict covers the merge gate at this exact head.
Summary
Adds
gr workspace init-from-topology <workspace-root>for a genuinely empty workspace that already carries neutralworkspace.tomlrepository declarations.The command lowers each declaration's
key,path, andurlinto the existing WorkspaceSpec writer. It preserves the existing scan-basedworkspace initcommand, performs no repository creation or Git initialization, and reportssource = workspace.tomlin both JSON and human-readable output.Premium boundary: gitgrip is OSS. This command is a workspace-materialization primitive: it lowers an already-declared topology into a workspace spec, authors no declaration, and resolves no identity.
Verification